MaskRay added a comment.
I am supportive of getting rid of InlineAsmDiagnosticHandler, too.
The updated AMDGPU tests suggeste that previously `MCContext::reportError` may
be called with no `SrcMgr` or `InlineSrcMgr`, so the error is propagated to the
temporary `SourceMgr()`. The `LLCDiagnosticHandler` does not receive anything
so the exit code is incorrect 0.
The new behavior moves the `SrcMgr` or `InlineSrcMgr` check under `if
(Loc.isValid()) {` (in `MCContext::reportCommon`). There is still a temporary
`SrcMgr`. I wonder whether this can be improved.
================
Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1036
+ void print(DiagnosticPrinter &DP) const override {
+ llvm_unreachable("unimplemented");
+ }
----------------
@nickdesaulniers's diagnostic means this is reachable.
Perhaps `Diagnostic.print` needs to be called with appropriate arguments.
================
Comment at: llvm/lib/MC/MCContext.cpp:869
+ SMLoc Loc,
+ std::function<void(SMDiagnostic &, const SourceMgr *)> GetMessage) {
+ SourceMgr SM;
----------------
Use lightweight function_ref since you don't need to store the callback.
================
Comment at: llvm/lib/MC/MCContext.cpp:870
+ std::function<void(SMDiagnostic &, const SourceMgr *)> GetMessage) {
+ SourceMgr SM;
+ const SourceMgr *SMP = &SM;
----------------
This looks a bit strange: we need to construct a fresh SourceMgr to print a
diagnostic.
================
Comment at: llvm/test/CodeGen/AMDGPU/lds-initializer.ll:1-2
-; RUN: llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
-; RUN: llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
----------------
nickdesaulniers wrote:
> Does the addition of `not` mean that this test would have failed due to these
> changes? How come?
This is an improvement. Errors should return non-zero. It might be clear to
change the CHECK below to have `error:`.
In the new code, `LLCDiagnosticHandler` sets `HasError` to true.
================
Comment at: llvm/test/CodeGen/AMDGPU/lds-zero-initializer.ll:1
-; RUN: llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
-; RUN: llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tahiti < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: not llc -march=amdgcn -mcpu=tonga < %s -o /dev/null 2>&1 | FileCheck %s
----------------
While here, `-o /dev/null` -> `-filetype=null`
================
Comment at: llvm/test/CodeGen/AMDGPU/lds-zero-initializer.ll:4
; CHECK: lds: unsupported initializer for address space
----------------
Add `error:` to make it clear this is an error.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97449/new/
https://reviews.llvm.org/D97449
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
