[llc] set canonical triple in module - #203725
Conversation
|
Can we add regression tests for the different divergence scenarios? |
Done! It is somewhat tricky because there isn't a good way to match on the default triple but I think these at least confirm that something gets set in the module, which did not happen before. |
| ; REQUIRES: aarch64-registered-target | ||
| ; REQUIRES: default_triple |
| ; REQUIRES: aarch64-registered-target | ||
| ; REQUIRES: x86-registered-target |
| ; MARCH: target triple = "aarch64-unknown-linux-gnu" | ||
| ; MTRIPLE: target triple = "aarch64-unknown-unknown" | ||
| ; MODULE: target triple = "x86_64-unknown-linux-gnu" | ||
|
|
||
| target triple = "x86_64-unknown-linux-gnu" |
There was a problem hiding this comment.
I don't think this behavior makes any sense. Why should the command line override the triple in the file?
The command line triple is only really useful in tests where you want to have no triple in the file, and then test with multiple triples. I think it would be less surprising if this errored if the command line triple doesn't match the module triple.
Also, whatever the behavior is, it should match opt.
There was a problem hiding this comment.
The -mtriple command line flag has long overridden the triple set in the file in both opt and llc. I don't want to open the can of worms that changing that would be nor do I think it is directly related to this PR.
This change is just about making sure that irrespective of what precedence rules we use to choose the canonical triple, that triple is consistent across both the module and the TM. This is important to prevent crashes and mis-compilations. I'm specifically trying to fix the case where LLVM_DEFAULT_TARGET_TRIPLE is set to nvptx and a module doesn't have a triple. Currently we have lots of issues on this path because passes like ExpandVariadics and TargetLibraryInfo consult the (previously unset) module triple to decide what to do.
There was a problem hiding this comment.
Why should the command line override the triple in the file?
My general view is that more specific input should be able to override less specific one.
I.e. compiler default < something specified in the source file < something specified on command line.
The override may not always be valid, but that's a user problem. Nothing stops me trying to compile fortran source as C++ if I specify -x c++. It does not make a lot of sense, but it's not the reason to ignore explicit user input.
There was a problem hiding this comment.
These are not user facing tools. They exist solely for lit tests and developer experiments. Flexibility is a big minus, we should prefer to error on mismatches
There was a problem hiding this comment.
Diagnostics are fine when they make sense. I'm not sure what/how we'd diagnose here.
Flexibility is a big minus
I'm of the opposite view. IMO, it's the user-facing apps where we need strict validity checks, and it's development is where we can/should allow maximum flexibility and less guardrails. Presumably developers should know what they are doing with those tools, while the users operate on documentation-based contract (ish).
There was a problem hiding this comment.
I vaguely remember when we intentionally chose to have llc override the module triple. At the time, the use case was to be able to dump IR, compile it with llc, then compile it again with a new triple that overrides the module triple, perhaps with different ISA features, to observe the result.
I have never wanted to do this. IR is target specific from construction, and running another target's IR through llc is just looking for less-than-useful bug reports. This is only useful as a stochastic fuzzing technique for unexpected inputs. The realistic use case for triple-from-command-line is when you want to compile multiple targets in the same lit test, like x86 tests 32/64 bit case. In which case the file shouldn't contain a triple and llc should set it.
I think, to your point, llc is a developer tool, so why shouldn't llc -mtriple=x override the module triple? SInce we have that support, why shouldn't it rewrite the module triple? Why is flexibility bad for developer experience?
Because of the element of surprise. I do not expect an arbitrary IR sample to work across all targets, especially if that's IR from any real program.
However, I don't think it's reasonable to ask @AlexMaclean to make llc emit an error on triple mismatch, and then to clean up our entire lit test suite, add release notes, document user-visible breaking changes, etc, just to set the module triple for generic tests.
Doing that would be an improvement over the status quo, I don't think this is. The module's triple should be set, but not if it conflicts with the command line's. I also don't think this would be a particularly pervasive change, did you try?
This blocks #204637 since the CodeGen/Generic tests do not set the triple in the module and without this change the default triple is not set in the module either.
I didn't follow the specific here, but CodeGen/Generic tests simply shouldn't exist. You cannot have a stable test that works on an arbitrary backends, and this is a frequent source of build bot breakage. These should be sharded out into individual targets (though the common case here just needs to pick one as a sample).
There was a problem hiding this comment.
I have never wanted to do this. IR is target specific from construction, and running another target's IR through llc is just looking for less-than-useful bug reports. This is only useful as a stochastic fuzzing technique for unexpected inputs. The realistic use case for triple-from-command-line is when you want to compile multiple targets in the same lit test, like x86 tests 32/64 bit case. In which case the file shouldn't contain a triple and llc should set it.
...
I do not expect an arbitrary IR sample to work across all targets, especially if that's IR from any real program.
I think there's longstanding disagreement on this point. It is true that LLVM IR is not in general portable, it is a compiler IR (@sunfishcode ), but there is a portable core to it, and we have rejected proposals that would make LLVM IR lower-level (i.e. encoding which parameters are passed in memory in the frontend to memory optimizations) if it would make that core IR less portable.
I didn't follow the specific here, but CodeGen/Generic tests simply shouldn't exist. You cannot have a stable test that works on an arbitrary backends, and this is a frequent source of build bot breakage. These should be sharded out into individual targets (though the common case here just needs to pick one as a sample).
I think shouldn't exist goes too far. I think we could solve all the problems you mentioned with some lit templating logic to stamp out llc RUN lines for all registered targets with standard triples, with generated CHECK-${ARCH} lines.
I think that would actually be a substantial improvement over the current situation of needing to copy and maintain tests for generic features (i128, musttail, byval) over to every architecture and to keep them in sync. Improvements to test coverage of a generic feature end up carrying over to every backend. It's a problem that llvm/tests is 1GiB of generated code that produces brittle, unreviewable diffs, and we should try to do something about it.
There was a problem hiding this comment.
Doing that would be an improvement over the status quo, I don't think this is. The module's triple should be set, but not if it conflicts with the command line's. I also don't think this would be a particularly pervasive change, did you try?
I think this is an improvement over the status quo even assuming that we want your preferred behavior. It seems like we're agreed that the triple used to select the TM and the triple in the module should agree and that there are cases, such as when no triple is specified in the module, where we would want to set the module triple to whatever llc decides it should be. All this change does is ensure this stamping occurs. The logic for triple precedence/erroring remains the status quo.
I just gave it a try and there are hundreds of tests which would need to be updated. Most can probably be fixed by simply removing the triple in the module but some are more tricky such as this:
I'm sure there's a way to make them all pass but I don't really have the time or expertise to address them all and I don't think this change actually needs to expand to cover that. This is a 1-line targeted fix primarily for cases where the default triple is used. I don't think it makes sense to expand the scope to re-litigate design decisions in llc.
There was a problem hiding this comment.
This is a 1-line targeted fix primarily for cases where the default triple is used. I don't think it makes sense to expand the scope to re-litigate design decisions in llc.
I agree with @AlexMaclean on that.
There was a problem hiding this comment.
I just gave it a try and there are hundreds of tests which would need to be updated. Most can probably be fixed by simply removing the triple in the module but some are more tricky such as this:
That example would be solved by using isCompatibleWith instead of string equality
| if (!TargetTriple.empty()) | ||
| M->setTargetTriple(Triple(Triple::normalize(TargetTriple))); | ||
|
|
||
| M->setTargetTriple(TheTriple); |
There was a problem hiding this comment.
Stamp the canonical target triple into the module so that this divergence is no longer possible.
AFAICT, TheTriple is the default target triple, while TargetTriple is derived from the command line -mtriple. I think that in general command line options should be overriding the compiler defaults, or the value specified in the IR module.
I think the old code was doing the right thing.
Can you elaborate on specific examples of triple divergence you want to fix?
There was a problem hiding this comment.
AFAICT, TheTriple is the default target triple, while TargetTriple is derived from the command line -mtriple.
TheTriple is what gets used to create the TargetMachine so it is not the default target triple. It's the canonical triple accounting for everything. TargetTriple is just the string from the mtriple cl::opt.
llvm-project/llvm/tools/llc/llc.cpp
Lines 651 to 652 in 6f8ec43
Can you elaborate on specific examples of triple divergence you want to fix?
I'm specifically trying to fix the case where LLVM_DEFAULT_TARGET_TRIPLE is set to nvptx and a module doesn't have a triple. Currently we have lots of issues on this path because passes like ExpandVariadics and TargetLibraryInfo consult the (previously unset) module triple to decide what to do. This is why the llvm/test/CodeGen/Generic/intrinsics.ll test is fixed by this change.
There was a problem hiding this comment.
where LLVM_DEFAULT_TARGET_TRIPLE is set to nvptx and a module doesn't have a triple.
In that case, should't M->setTargetTriple(TheTriple) be guarded by the check on whether the module triple is empty?
There was a problem hiding this comment.
In that case, should't M->setTargetTriple(TheTriple) be guarded by the check on whether the module triple is empty?
The guard would need to be a more complex. We also want to update when !TargetTriple.empty() as the previous logic had. In addition we should probably consistently respect -march in both places. So the guard is "is TheTriple different from the module triple" we could check that directly but I don't see the point since there is no harm in unconditionally updating to the right triple, even if sometimes the update doesn't change anything.
There was a problem hiding this comment.
OK. I guess regardless of what the module's triple says, we're about to compile it with the current compiler triple. So that addresses the "should we set the triple unconditionally" part.
However, I'm not sure what's the right value to set it to. I still think that command-line should have precedence. Based on module-triple.ll test, it already seems to work that way. -mtriple=aarch64-unknown-unknowndoes set user-specified triple, but I'm not sure where/how that happens.
It may be worth adding a comment that this triple may be further overridden somewhere else (or, maybe, it gets derived from -mtriple before?).
There was a problem hiding this comment.
Based on module-triple.ll test, it already seems to work that way. -mtriple=aarch64-unknown-unknowndoes set user-specified triple, but I'm not sure where/how that happens.
llvm-project/llvm/tools/llc/llc.cpp
Lines 634 to 644 in 6f8ec43
Here is where TheTriple gets set, it checks IRTargetTriple from the module and overrides with TargetTriple (-mtriple) If that is non-empty. It finally falls back to the default if that initial triple is empty.
Artem-B
left a comment
Artem-B
left a comment
There was a problem hiding this comment.
LGTM in principle. I'm OK deferring final approval to Matt.
| if (!TargetTriple.empty()) | ||
| M->setTargetTriple(Triple(Triple::normalize(TargetTriple))); | ||
|
|
||
| M->setTargetTriple(TheTriple); |
There was a problem hiding this comment.
OK. I guess regardless of what the module's triple says, we're about to compile it with the current compiler triple. So that addresses the "should we set the triple unconditionally" part.
However, I'm not sure what's the right value to set it to. I still think that command-line should have precedence. Based on module-triple.ll test, it already seems to work that way. -mtriple=aarch64-unknown-unknowndoes set user-specified triple, but I'm not sure where/how that happens.
It may be worth adding a comment that this triple may be further overridden somewhere else (or, maybe, it gets derived from -mtriple before?).
|
Code should also really be taking the triple from the module. What contexts are reading it from the TargetMachine? |
The problem is not that the triple is being read from the TargetMachine. I'll expand on my example from last month to hopefully clarify any confusion about what problem is being addressed.
Here is the situation:
It seems like all the debate is around how step 3 chooses a triple while all my change actually does is make it so that step 4 also puts the triple in the module. @arsenm Do you agree that a case like this is a problem? |
|
I see, the updated commit message makes the change much clearer. Also, reading the code helps. I think the behavior that @arsenm wants to remove (command line triple overrides module triple) is really orthogonal to what this change is doing. This change fixes a real bug today: When the default triple is used, we don't stamp it into the module, but we should. When an explicit triple is used, we already stamp it into the module. |
rnk
left a comment
rnk
left a comment
There was a problem hiding this comment.
apparently this comment got hung up, I didn't hit send
| ; MARCH: target triple = "aarch64-unknown-linux-gnu" | ||
| ; MTRIPLE: target triple = "aarch64-unknown-unknown" | ||
| ; MODULE: target triple = "x86_64-unknown-linux-gnu" | ||
|
|
||
| target triple = "x86_64-unknown-linux-gnu" |
There was a problem hiding this comment.
I have never wanted to do this. IR is target specific from construction, and running another target's IR through llc is just looking for less-than-useful bug reports. This is only useful as a stochastic fuzzing technique for unexpected inputs. The realistic use case for triple-from-command-line is when you want to compile multiple targets in the same lit test, like x86 tests 32/64 bit case. In which case the file shouldn't contain a triple and llc should set it.
...
I do not expect an arbitrary IR sample to work across all targets, especially if that's IR from any real program.
I think there's longstanding disagreement on this point. It is true that LLVM IR is not in general portable, it is a compiler IR (@sunfishcode ), but there is a portable core to it, and we have rejected proposals that would make LLVM IR lower-level (i.e. encoding which parameters are passed in memory in the frontend to memory optimizations) if it would make that core IR less portable.
I didn't follow the specific here, but CodeGen/Generic tests simply shouldn't exist. You cannot have a stable test that works on an arbitrary backends, and this is a frequent source of build bot breakage. These should be sharded out into individual targets (though the common case here just needs to pick one as a sample).
I think shouldn't exist goes too far. I think we could solve all the problems you mentioned with some lit templating logic to stamp out llc RUN lines for all registered targets with standard triples, with generated CHECK-${ARCH} lines.
I think that would actually be a substantial improvement over the current situation of needing to copy and maintain tests for generic features (i128, musttail, byval) over to every architecture and to keep them in sync. Improvements to test coverage of a generic feature end up carrying over to every backend. It's a problem that llvm/tests is 1GiB of generated code that produces brittle, unreviewable diffs, and we should try to do something about it.
There was a problem hiding this comment.
LGTM + reverse ping. This fixes the RISC-V buildbots (https://igalia.github.io/riscv-llvm-ci/) after #215796
6f8ec43 to
0edad95
Compare
|
Based on approvals it looks like there is consensus to land. I had to remove the |
|
@llvm/pr-subscribers-backend-x86 Author: Alex MacLean (AlexMaclean) ChangesPrior to this change the effective triple for the TargetMachine and the triple in the module were determined by different criteria and diverged in some cases. This can create lots of issues in passes which check the module triple. TargetMachine:
Module:
Stamp the canonical target triple into the module so that this divergence is no longer possible. Full diff: https://github.com/llvm/llvm-project/pull/203725.diff 4 Files Affected:
diff --git a/llvm/test/CodeGen/X86/expand-frem-no-libcall.ll b/llvm/test/CodeGen/X86/expand-frem-no-libcall.ll
deleted file mode 100644
index c29b072d96e8e..0000000000000
--- a/llvm/test/CodeGen/X86/expand-frem-no-libcall.ll
+++ /dev/null
@@ -1,31 +0,0 @@
-; This test underhandedly exploits a bug in llc's handling of -march.
-; The module ends up with no triple, and thus treated as unknown arch
-; with no library functions.
-
-; RUN: llc -march=x86-64 -stop-after=expand-ir-insts %s -o - | FileCheck --check-prefix=EXPAND %s
-; RUN: llc -mtriple=x86_64-linux-gnu -stop-after=expand-ir-insts %s -o - | FileCheck --check-prefix=LIBCALL %s
-
-; When the fmod libcall is unavailable, expand-ir-insts must expand
-; frem inline instead of leaving it for the DAG legalizer.
-
-; EXPAND-LABEL: define float @frem_f32
-; EXPAND-NOT: frem float
-; EXPAND: fmul float
-
-; LIBCALL-LABEL: define float @frem_f32
-; LIBCALL: frem float
-define float @frem_f32(float %a, float %b) {
- %r = frem float %a, %b
- ret float %r
-}
-
-; EXPAND-LABEL: define double @frem_f64
-; EXPAND-NOT: frem double
-; EXPAND: fmul double
-
-; LIBCALL-LABEL: define double @frem_f64
-; LIBCALL: frem double
-define double @frem_f64(double %a, double %b) {
- %r = frem double %a, %b
- ret double %r
-}
diff --git a/llvm/test/tools/llc/module-triple-default.ll b/llvm/test/tools/llc/module-triple-default.ll
new file mode 100644
index 0000000000000..161d76557450a
--- /dev/null
+++ b/llvm/test/tools/llc/module-triple-default.ll
@@ -0,0 +1,14 @@
+; REQUIRES: aarch64-registered-target
+; REQUIRES: default_triple
+
+;; Verify that llc correctly sets the module triple when one is not present.
+
+; RUN: llc -march=aarch64 -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=MARCH
+; RUN: llc -mtriple=aarch64-unknown-linux-gnu -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=MTRIPLE
+; RUN: llc -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=DEFAULT
+
+; MARCH: target triple = "aarch64-{{.*}}-{{.*}}"
+; MTRIPLE: target triple = "aarch64-unknown-linux-gnu"
+; DEFAULT: target triple = "{{.*}}-{{.*}}-{{.*}}"
+
+define void @f() { ret void }
diff --git a/llvm/test/tools/llc/module-triple.ll b/llvm/test/tools/llc/module-triple.ll
new file mode 100644
index 0000000000000..6e26babbda080
--- /dev/null
+++ b/llvm/test/tools/llc/module-triple.ll
@@ -0,0 +1,16 @@
+; REQUIRES: aarch64-registered-target
+; REQUIRES: x86-registered-target
+
+;; Verify that llc correctly sets the module triple when one is present.
+
+; RUN: llc -march=aarch64 -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=MARCH
+; RUN: llc -mtriple=aarch64-unknown-unknown -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=MTRIPLE
+; RUN: llc -stop-after=finalize-isel -o - %s | FileCheck %s --check-prefix=MODULE
+
+; MARCH: target triple = "aarch64-unknown-linux-gnu"
+; MTRIPLE: target triple = "aarch64-unknown-unknown"
+; MODULE: target triple = "x86_64-unknown-linux-gnu"
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @f() { ret void }
diff --git a/llvm/tools/llc/llc.cpp b/llvm/tools/llc/llc.cpp
index 4d00e0fcb048a..2c2a543862a28 100644
--- a/llvm/tools/llc/llc.cpp
+++ b/llvm/tools/llc/llc.cpp
@@ -675,8 +675,8 @@ static int compileModule(char **argv, SmallVectorImpl<PassPlugin> &PluginList,
Err.print(argv[0], WithColor::error(errs(), argv[0]));
return 1;
}
- if (!TargetTriple.empty())
- M->setTargetTriple(Triple(Triple::normalize(TargetTriple)));
+
+ M->setTargetTriple(TheTriple);
std::optional<CodeModel::Model> CM_IR = M->getCodeModel();
if (!CM && CM_IR)
|
6 participants
Prior to this change the effective triple for the TargetMachine and the triple in the module were determined by different criteria and diverged in some cases. This can create lots of issues in passes which check the module triple.
TargetMachine:
Module:
Stamp the canonical target triple into the module so that this divergence is no longer possible.