[RISCV] Do not re-validate target ABI in AsmParser if already resolved by streamer - #223606
Conversation
|
@llvm/pr-subscribers-lld-elf Author: Alexander Richardson (arichardson) ChangesCommit 105ff16 (#213410) However, this broke LTO builds containing inline assembly (e.g. Android To fix this, skip the parser validation whenever the streamer has This commit was created with the help of AI tools Full diff: https://github.com/llvm/llvm-project/pull/223606.diff 5 Files Affected:
diff --git a/lld/test/ELF/lto/riscv-target-abi.ll b/lld/test/ELF/lto/riscv-target-abi.ll
index 07fdf122cb0c48..fdb93b82fc41e0 100644
--- a/lld/test/ELF/lto/riscv-target-abi.ll
+++ b/lld/test/ELF/lto/riscv-target-abi.ll
@@ -28,7 +28,14 @@
target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
target triple = "riscv64"
+module asm "nop"
+;; Module asm with target features not including 'd' (would fail before fix)
+module asm(target_features: "+c") "c.nop"
+;; Module asm with target features enabling 'd'
+module asm(target_features: "+d") "fld f0, 0(sp)"
+
define void @_start() {
+ call void asm sideeffect "nop", ""()
ret void
}
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 4f1f3cbfb9b26c..d18f7c7416ac67 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -343,10 +343,21 @@ class RISCVAsmParser : public MCTargetAsmParser {
// with the first token, so diagnostics can be reported with a real source
// location instead of being printed with no location information.
void onBeginOfFile() override {
+ // If the target streamer already has a resolved ABI (e.g. set by
+ // RISCVTargetELFStreamer for a valid -target-abi, or set by
+ // RISCVAsmPrinter during codegen), skip validation.
+ if (getTargetStreamer().hasTargetABI())
+ return;
+
Expected<RISCVABI::ABI> ABIOrErr =
RISCVABI::computeTargetABI(getSTI(), getTargetOptions().ABIName);
- if (!ABIOrErr)
+ if (!ABIOrErr) {
getParser().printError(getLoc(), toString(ABIOrErr.takeError()));
+ getTargetStreamer().setTargetABI(
+ cantFail(RISCVABI::computeTargetABI(getSTI(), "")));
+ return;
+ }
+ getTargetStreamer().setTargetABI(*ABIOrErr);
}
};
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 049123068070f1..1fb45627a1d03f 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -40,12 +40,10 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
if (auto ABIOrErr = RISCVABI::computeTargetABI(STI, ABIName)) {
setTargetABI(*ABIOrErr);
} else {
- // Do not warn here and instead silently fall back to the default ABI:
- // either RISCVSubtarget::initializeSubtargetDependencies() or
- // RISCVAsmParser::onBeginOfFile() will print the message with proper
- // contexts. Reporting here would just duplicate that diagnostic.
+ // Do not set TargetABI here if invalid: RISCVSubtarget/RISCVAsmPrinter
+ // (in codegen) or RISCVAsmParser::onBeginOfFile() (in llvm-mc) will
+ // resolve or diagnose it with proper contexts.
consumeError(ABIOrErr.takeError());
- setTargetABI(cantFail(RISCVABI::computeTargetABI(STI, "")));
}
setFlagsFromFeatures(STI);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
index 0601cf0ed0362a..cd59089fae4c7e 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
@@ -63,6 +63,7 @@ class RISCVTargetStreamer : public MCTargetStreamer {
void emitTargetAttributes(const MCSubtargetInfo &STI, bool EmitStackAlign);
void setTargetABI(RISCVABI::ABI ABI);
RISCVABI::ABI getTargetABI() const { return TargetABI; }
+ bool hasTargetABI() const { return TargetABI != RISCVABI::ABI_Unknown; }
void setFlagsFromFeatures(const MCSubtargetInfo &STI);
bool hasRVC() const { return HasRVC; }
bool hasTSO() const { return HasTSO; }
diff --git a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
index b535404d25c1f0..57fb1dfc48fecc 100644
--- a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
+++ b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
@@ -649,6 +649,8 @@ void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
if (const MDString *ModuleTargetABI =
dyn_cast_or_null<MDString>(M.getModuleFlag("target-abi")))
RTS.setTargetABI(RISCVABI::getTargetABI(ModuleTargetABI->getString()));
+ else if (!RTS.hasTargetABI())
+ RTS.setTargetABI(TM.getSubtarget<RISCVSubtarget>().getTargetABI());
MCSubtargetInfo SubtargetInfo = TM.getMCSubtargetInfo();
|
|
@llvm/pr-subscribers-backend-risc-v Author: Alexander Richardson (arichardson) ChangesCommit 105ff16 (#213410) However, this broke LTO builds containing inline assembly (e.g. Android To fix this, skip the parser validation whenever the streamer has This commit was created with the help of AI tools Full diff: https://github.com/llvm/llvm-project/pull/223606.diff 5 Files Affected:
diff --git a/lld/test/ELF/lto/riscv-target-abi.ll b/lld/test/ELF/lto/riscv-target-abi.ll
index 07fdf122cb0c4..fdb93b82fc41e 100644
--- a/lld/test/ELF/lto/riscv-target-abi.ll
+++ b/lld/test/ELF/lto/riscv-target-abi.ll
@@ -28,7 +28,14 @@
target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
target triple = "riscv64"
+module asm "nop"
+;; Module asm with target features not including 'd' (would fail before fix)
+module asm(target_features: "+c") "c.nop"
+;; Module asm with target features enabling 'd'
+module asm(target_features: "+d") "fld f0, 0(sp)"
+
define void @_start() {
+ call void asm sideeffect "nop", ""()
ret void
}
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 4f1f3cbfb9b26..d18f7c7416ac6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -343,10 +343,21 @@ class RISCVAsmParser : public MCTargetAsmParser {
// with the first token, so diagnostics can be reported with a real source
// location instead of being printed with no location information.
void onBeginOfFile() override {
+ // If the target streamer already has a resolved ABI (e.g. set by
+ // RISCVTargetELFStreamer for a valid -target-abi, or set by
+ // RISCVAsmPrinter during codegen), skip validation.
+ if (getTargetStreamer().hasTargetABI())
+ return;
+
Expected<RISCVABI::ABI> ABIOrErr =
RISCVABI::computeTargetABI(getSTI(), getTargetOptions().ABIName);
- if (!ABIOrErr)
+ if (!ABIOrErr) {
getParser().printError(getLoc(), toString(ABIOrErr.takeError()));
+ getTargetStreamer().setTargetABI(
+ cantFail(RISCVABI::computeTargetABI(getSTI(), "")));
+ return;
+ }
+ getTargetStreamer().setTargetABI(*ABIOrErr);
}
};
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 049123068070f..1fb45627a1d03 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -40,12 +40,10 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
if (auto ABIOrErr = RISCVABI::computeTargetABI(STI, ABIName)) {
setTargetABI(*ABIOrErr);
} else {
- // Do not warn here and instead silently fall back to the default ABI:
- // either RISCVSubtarget::initializeSubtargetDependencies() or
- // RISCVAsmParser::onBeginOfFile() will print the message with proper
- // contexts. Reporting here would just duplicate that diagnostic.
+ // Do not set TargetABI here if invalid: RISCVSubtarget/RISCVAsmPrinter
+ // (in codegen) or RISCVAsmParser::onBeginOfFile() (in llvm-mc) will
+ // resolve or diagnose it with proper contexts.
consumeError(ABIOrErr.takeError());
- setTargetABI(cantFail(RISCVABI::computeTargetABI(STI, "")));
}
setFlagsFromFeatures(STI);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
index 0601cf0ed0362..cd59089fae4c7 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
@@ -63,6 +63,7 @@ class RISCVTargetStreamer : public MCTargetStreamer {
void emitTargetAttributes(const MCSubtargetInfo &STI, bool EmitStackAlign);
void setTargetABI(RISCVABI::ABI ABI);
RISCVABI::ABI getTargetABI() const { return TargetABI; }
+ bool hasTargetABI() const { return TargetABI != RISCVABI::ABI_Unknown; }
void setFlagsFromFeatures(const MCSubtargetInfo &STI);
bool hasRVC() const { return HasRVC; }
bool hasTSO() const { return HasTSO; }
diff --git a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
index b535404d25c1f..57fb1dfc48fec 100644
--- a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
+++ b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
@@ -649,6 +649,8 @@ void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
if (const MDString *ModuleTargetABI =
dyn_cast_or_null<MDString>(M.getModuleFlag("target-abi")))
RTS.setTargetABI(RISCVABI::getTargetABI(ModuleTargetABI->getString()));
+ else if (!RTS.hasTargetABI())
+ RTS.setTargetABI(TM.getSubtarget<RISCVSubtarget>().getTargetABI());
MCSubtargetInfo SubtargetInfo = TM.getMCSubtargetInfo();
|
|
@llvm/pr-subscribers-lld Author: Alexander Richardson (arichardson) ChangesCommit 105ff16 (#213410) However, this broke LTO builds containing inline assembly (e.g. Android To fix this, skip the parser validation whenever the streamer has This commit was created with the help of AI tools Full diff: https://github.com/llvm/llvm-project/pull/223606.diff 5 Files Affected:
diff --git a/lld/test/ELF/lto/riscv-target-abi.ll b/lld/test/ELF/lto/riscv-target-abi.ll
index 07fdf122cb0c4..fdb93b82fc41e 100644
--- a/lld/test/ELF/lto/riscv-target-abi.ll
+++ b/lld/test/ELF/lto/riscv-target-abi.ll
@@ -28,7 +28,14 @@
target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
target triple = "riscv64"
+module asm "nop"
+;; Module asm with target features not including 'd' (would fail before fix)
+module asm(target_features: "+c") "c.nop"
+;; Module asm with target features enabling 'd'
+module asm(target_features: "+d") "fld f0, 0(sp)"
+
define void @_start() {
+ call void asm sideeffect "nop", ""()
ret void
}
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 4f1f3cbfb9b26..d18f7c7416ac6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -343,10 +343,21 @@ class RISCVAsmParser : public MCTargetAsmParser {
// with the first token, so diagnostics can be reported with a real source
// location instead of being printed with no location information.
void onBeginOfFile() override {
+ // If the target streamer already has a resolved ABI (e.g. set by
+ // RISCVTargetELFStreamer for a valid -target-abi, or set by
+ // RISCVAsmPrinter during codegen), skip validation.
+ if (getTargetStreamer().hasTargetABI())
+ return;
+
Expected<RISCVABI::ABI> ABIOrErr =
RISCVABI::computeTargetABI(getSTI(), getTargetOptions().ABIName);
- if (!ABIOrErr)
+ if (!ABIOrErr) {
getParser().printError(getLoc(), toString(ABIOrErr.takeError()));
+ getTargetStreamer().setTargetABI(
+ cantFail(RISCVABI::computeTargetABI(getSTI(), "")));
+ return;
+ }
+ getTargetStreamer().setTargetABI(*ABIOrErr);
}
};
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 049123068070f..1fb45627a1d03 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -40,12 +40,10 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
if (auto ABIOrErr = RISCVABI::computeTargetABI(STI, ABIName)) {
setTargetABI(*ABIOrErr);
} else {
- // Do not warn here and instead silently fall back to the default ABI:
- // either RISCVSubtarget::initializeSubtargetDependencies() or
- // RISCVAsmParser::onBeginOfFile() will print the message with proper
- // contexts. Reporting here would just duplicate that diagnostic.
+ // Do not set TargetABI here if invalid: RISCVSubtarget/RISCVAsmPrinter
+ // (in codegen) or RISCVAsmParser::onBeginOfFile() (in llvm-mc) will
+ // resolve or diagnose it with proper contexts.
consumeError(ABIOrErr.takeError());
- setTargetABI(cantFail(RISCVABI::computeTargetABI(STI, "")));
}
setFlagsFromFeatures(STI);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
index 0601cf0ed0362..cd59089fae4c7 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.h
@@ -63,6 +63,7 @@ class RISCVTargetStreamer : public MCTargetStreamer {
void emitTargetAttributes(const MCSubtargetInfo &STI, bool EmitStackAlign);
void setTargetABI(RISCVABI::ABI ABI);
RISCVABI::ABI getTargetABI() const { return TargetABI; }
+ bool hasTargetABI() const { return TargetABI != RISCVABI::ABI_Unknown; }
void setFlagsFromFeatures(const MCSubtargetInfo &STI);
bool hasRVC() const { return HasRVC; }
bool hasTSO() const { return HasTSO; }
diff --git a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
index b535404d25c1f..57fb1dfc48fec 100644
--- a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
+++ b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
@@ -649,6 +649,8 @@ void RISCVAsmPrinter::emitStartOfAsmFile(Module &M) {
if (const MDString *ModuleTargetABI =
dyn_cast_or_null<MDString>(M.getModuleFlag("target-abi")))
RTS.setTargetABI(RISCVABI::getTargetABI(ModuleTargetABI->getString()));
+ else if (!RTS.hasTargetABI())
+ RTS.setTargetABI(TM.getSubtarget<RISCVSubtarget>().getTargetABI());
MCSubtargetInfo SubtargetInfo = TM.getMCSubtargetInfo();
|
…d by streamer Commit 105ff16 (llvm#213410) changed RISCVABI::computeTargetABI() to return Expected<ABI> and added validation to RISCVAsmParser::onBeginOfFile() to report invalid -target-abi flags with real source location in llvm-mc. However, this broke LTO builds containing inline assembly (e.g. Android riscv64 builds and downstream Rust in rust-lang/rust#162783). During LTO, LLD sets TargetOptions.MCOptions.ABIName from the module's target-abi metadata ("lp64d"), while the linker's default TargetMachine subtarget lacks "+d" (individual functions specify "+d" in target-features). RISCVSubtarget handles this gracefully by emitting a diagnostic note and falling back to lp64 for code generation. When inline assembly was subsequently parsed, AsmPrinter::emitInlineAsm instantiated RISCVAsmParser with a subtarget lacking "+d", causing onBeginOfFile() to re-validate TargetOptions.ABIName and fail with a fatal error. To fix this, skip the parser validation whenever the streamer has already resolved an ABI. This preserves llvm-mc diagnostics on invalid command-line flags while avoiding conflicting validation when assembling inline asm during code generation. This commit was created with the help of AI tools
3fd95c8 to
b6a1130
Compare
|
cc @llvm/android-maintainers |
Sharjeel-Khan
left a comment
Sharjeel-Khan
left a comment
There was a problem hiding this comment.
Looks good from Android side
|
I'll wait until tomorrow to give others a chance to comment on this. Since it's breaking build bots and just restores the prior behaviour, I'll land it then unless there are any objections. |
I don't understand how this could happen. Either all the global inline assembly fragments have If there's one without, then surely the error message is correct? Or maybe we shouldn't care because target-abi doesn't matter for assembly in the same way. |
|
That is a good point maybe the problem is that Clang is not adding the needed target features to the global ASM fragments. But I'm a bit wary of changing that here since there could be more side effects for other targets. I can try to look into this as a follow up since it should definitely be beneficial for RVY |
|
I think clang got updated to correctly add this info when generating LLVM IR (when this was introduced) But I realise that we're also talking about Rust, which won't share that code. Is Rust adding this information to module-level inline asm? |
Based on the reproducer just being a global asm Not quite sure what rust does, maybe @durin42 / @RalfJung can comment. |
Clang does emit them: https://clang.godbolt.org/z/4crW4je3r Maybe this is mixing bitcode from older clang versions? Rust also emits the target features, though not on stable (which predates support for this). |
|
It seems to be caused by the Clang driver invoking ld.lld with |
|
Is it possible we land this for now to unblock Android's RISCV builders? They have been down since the weekend when the previous PR landed. A more proper fix can be made later to fix the clang driver's decision invoking with ld.lld with no CPU features. |
|
Sure |
|
Thanks for approving this temporary fix. I am currently working on a more comprehensive cleanup which turns out to be quite a rabbit hole... Hopefully will have something soon. |
6 participants
Commit 105ff16 (#213410)
changed RISCVABI::computeTargetABI() to return Expected and added
validation to RISCVAsmParser::onBeginOfFile() to report invalid
-target-abi flags with real source location in llvm-mc.
However, this broke LTO builds containing inline assembly (e.g. Android
riscv64 builds and downstream Rust in
rust-lang/rust#162783). During LTO, LLD sets
TargetOptions.MCOptions.ABIName from the module's target-abi metadata
("lp64d"), while the linker's default TargetMachine subtarget lacks
"+d" (individual functions specify "+d" in target-features).
RISCVSubtarget handles this gracefully by emitting a diagnostic note
and falling back to lp64 for code generation. When inline assembly was
subsequently parsed, AsmPrinter::emitInlineAsm instantiated
RISCVAsmParser with a subtarget lacking "+d", causing onBeginOfFile() to
re-validate TargetOptions.ABIName and fail with a fatal error.
To fix this, skip the parser validation whenever the streamer has
already resolved an ABI. This preserves llvm-mc diagnostics on invalid
command-line flags while avoiding conflicting validation when assembling
inline asm during code generation.
This commit was created with the help of AI tools