Skip to content

[Clang][DirectX] Always use Diagnostic Printer #135655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[Clang][DirectX] Always use Diagnostic Printer #135655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ec850f
[Clang][DirectX] Always use Diagnostic Printer
farzonl Apr 14, 2025
4b9619c
update test cases to new error format and restrict to correct target
farzonl Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions clang/lib/CodeGen/CodeGenAction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,13 +605,11 @@ void BackendConsumer::UnsupportedDiagHandler(

// Context will be nullptr for IR input files, we will construct the diag
// message from llvm::DiagnosticInfoUnsupported.
if (Context != nullptr) {
if (Context != nullptr)
Loc = getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the best location always "unknown"? or are these tests only for cases where the context is null?

MsgStream << D.getMessage();
} else {
DiagnosticPrinterRawOStream DP(MsgStream);
D.print(DP);
}

DiagnosticPrinterRawOStream DP(MsgStream);
D.print(DP);

auto DiagType = D.getSeverity() == llvm::DS_Error
? diag::err_fe_backend_unsupported
Expand Down
7 changes: 7 additions & 0 deletions clang/test/CodeGenDirectX/unsupported_reduc_mul.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// RUN: %if clang-dxc %{not %clang_dxc -T lib_6_3 %s 2>&1 | FileCheck %s %}

// CHECK: error: <unknown>:0:0: in function llvm.vector.reduce.mul.v4i32 i32 (<4 x i32>): Unsupported intrinsic for DXIL lowering

export int vecReduceMulTest(int4 vec) {
return __builtin_reduce_mul(vec);
}
3 changes: 1 addition & 2 deletions clang/test/Sema/builtin-amdgcn-fence-failure.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// RUN: not %clang_cc1 %s -o - -S -triple=amdgcn-amd-amdhsa 2>&1 | FileCheck %s

void test_amdgcn_fence_failure() {

// CHECK: error: Unsupported atomic synchronization scope
// CHECK: error: <unknown>:0:0: in function _Z25test_amdgcn_fence_failurev void (): Unsupported atomic synchronization scope
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current diagnostic without debug info:

<stdin>:1:6: error: Unsupported atomic synchronization scope
    1 | void test_amdgcn_fence_failure() {
      |      ^

Current diagnostic with debug info:

<stdin>:3:3: error: Unsupported atomic synchronization scope
    3 |   __builtin_amdgcn_fence(__ATOMIC_SEQ_CST, "foobar");
      |   ^

The one without debug info could maybe be improved a bit, but throwing away the location info doesn't seem like the right approach.

Copy link
Member Author

@farzonl farzonl Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My change doesn't get rid of the location we still prefix it, but <unknown>:0:0 is also prefixed and that I agree is problematic. I suppose this wasn't a problem for the Context==nullptr case because in that on Diags.Report(Loc, DiagType) might be an empty string?

Before change No debug:

<llvm-project_dir>/clang/test/Sema/builtin-amdgcn-fence-failure.cpp:4:6: error: Unsupported atomic synchronization scope
   4 | void test_amdgcn_fence_failure() {
      |      ^

Before change debug

/mnt/DevDrive/projects/llvm-project/clang/test/Sema/builtin-amdgcn-fence-failure.cpp:7:3: error: Unsupported atomic synchronization scope
    7 |   __builtin_amdgcn_fence(__ATOMIC_SEQ_CST, "foobar");
      |   ^

After change no debug:

<llvm-project_dir>/clang/test/Sema/builtin-amdgcn-fence-failure.cpp:4:6: error: <unknown>:0:0: in function _Z25test_amdgcn_fence_failurev void (): Unsupported atomic synchronization scope
    7 |   __builtin_amdgcn_fence(__ATOMIC_SEQ_CST, "foobar");
      |   ^

After change debug

/mnt/DevDrive/projects/llvm-project/clang/test/Sema/builtin-amdgcn-fence-failure.cpp:7:3: error: clang/test/Sema/builtin-amdgcn-fence-failure.cpp:7:3: in function _Z25test_amdgcn_fence_failurev void (): Unsupported atomic synchronization scope
    7 |   __builtin_amdgcn_fence(__ATOMIC_SEQ_CST, "foobar");
      |   ^

__builtin_amdgcn_fence(__ATOMIC_SEQ_CST, "foobar");
}
Loading