[llvm] [llc] set canonical triple in module (PR #203725)

[Reid Kleckner via llvm-commits]

================
@@ -0,0 +1,16 @@
+;; 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
+
+; 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"
----------------
rnk wrote:

> 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

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.

Or, at least I have some vague recollection of Jim Grosbach making this case 10 years ago. Perhaps now all this microarch support is encoded in function attributes, and that use case has rotted.

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?

> Either way, this patch doesn't change datalayout handling. llc already overrides the module's DL with the target's during parsing, so the DL is always whatever the target dictates, independent of this change.

I wonder if that changed at some point. It begs the question what purpose serializing the datalayout even serves at this point. Back in the day, the point was to enable libraries to optimize IR without directly querying target libraries for pointer size and `i64` alignment. I moved that info out of lib/Target/* and into lib/TargetParser, so you don't need a registered target to query datalayout from the triple.

In conclusion, we should probably do nothing and leave the vestigial datalayout serialization to avoid churn. Removing it would be more trouble than it's worth.

> This blocks https://github.com/llvm/llvm-project/pull/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. This causes the ExpandVariadics pass not to fire breaking assumptions in ISel.

This seems like the clear reason why we should do this, this is the use case we agree on: Setting the module triple when the module triple is unset / non-conflicting.

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.

Possible compromises:
1. Have llc emit a warning on non-empty conflicting module triples. This is also more churn than I'd like.
2. Have llc only set the triple for modules with no triple. This is subtle.

---

On balance, I think we should go with the original change.

https://github.com/llvm/llvm-project/pull/203725