Skip to content

[llc] set canonical triple in module #203725

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

Merged
arsenm merged 4 commits into from
Aug 20, 2026
Merged

[llc] set canonical triple in module #203725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c0c9fc5
[llc] set canonical triple in module
AlexMaclean Jun 13, 2026
b648ae7
add tests
AlexMaclean Jun 14, 2026
0edad95
address nits
AlexMaclean Jun 15, 2026
fce74b5
remove bad test
AlexMaclean Aug 20, 2026
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
31 changes: 0 additions & 31 deletions llvm/test/CodeGen/X86/expand-frem-no-libcall.ll
View file
Open in desktop

This file was deleted.

14 changes: 14 additions & 0 deletions llvm/test/tools/llc/module-triple-default.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 }
16 changes: 16 additions & 0 deletions llvm/test/tools/llc/module-triple.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Comment on lines +10 to +14

@arsenm arsenm Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@AlexMaclean AlexMaclean Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@Artem-B Artem-B Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@arsenm arsenm Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Artem-B Artem-B Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@arsenm arsenm Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@rnk rnk Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@AlexMaclean AlexMaclean Jul 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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:

llvm-project/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id.ll

Lines 1 to 7 in d982e04

; RUN: opt -mtriple=amdgpu-- -passes=amdgpu-attributor -o %t.bc %s
; RUN: llc -mtriple=amdgpu7.01 < %t.bc | FileCheck --check-prefixes=ALL,MESA,UNPACKED %s
; RUN: llc -mtriple=amdgpu8.02 -mattr=-flat-for-global < %t.bc | FileCheck --check-prefixes=ALL,MESA,UNPACKED %s
; RUN: llc -mtriple=amdgpu7.01-unknown-mesa3d < %t.bc | FileCheck -check-prefixes=ALL,MESA3D,UNPACKED %s
; RUN: llc -mtriple=amdgpu8.02-unknown-mesa3d -mattr=-flat-for-global < %t.bc | FileCheck -check-prefixes=ALL,MESA3D,UNPACKED %s
; RUN: llc -mtriple=amdgpu9.0a-unknown-amdhsa < %t.bc | FileCheck -check-prefixes=ALL,PACKED-TID %s
; RUN: llc -mtriple=amdgpu11.00-unknown-amdhsa -amdgpu-enable-vopd=0 < %t.bc | FileCheck -check-prefixes=ALL,PACKED-TID %s

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.

@Artem-B Artem-B Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@arsenm arsenm Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


define void @f() { ret void }
4 changes: 2 additions & 2 deletions llvm/tools/llc/llc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@Artem-B Artem-B Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@AlexMaclean AlexMaclean Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Target = std::unique_ptr<TargetMachine>(TheTarget->createTargetMachine(
TheTriple, CPUStr, FeaturesStr, Options, RM, CM, OLvl));

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.

@Artem-B Artem-B Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@AlexMaclean AlexMaclean Jun 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

@Artem-B Artem-B Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?).

@AlexMaclean AlexMaclean Jun 16, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

// If we are supposed to override the target triple, do so now.
std::string IRTargetTriple = DataLayoutTargetTriple.str();
if (!TargetTriple.empty())
IRTargetTriple = Triple::normalize(TargetTriple);
TheTriple = Triple(IRTargetTriple);
if (TheTriple.getTriple().empty())
TheTriple.setTriple(sys::getDefaultTargetTriple());
std::string Error;
TheTarget =
TargetRegistry::lookupTarget(codegen::getMArch(), TheTriple, Error);

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.


std::optional<CodeModel::Model> CM_IR = M->getCodeModel();
if (!CM && CM_IR)
Expand Down
Loading