Skip to content

Conversation

@arsenm
Copy link
Contributor

@arsenm arsenm commented Nov 26, 2025

The Triple directly has the datalayout string in it, so just
use that.

The logical flow here is kind of a mess. We were constructing
a temporary target machine in the asm parser to infer the datalayout,
throwing it away, and then creating another target machine for the
actual compilation. The flow of the Triple construction is still
convoluted, but we can at least drop the TargetMachine.

The Triple directly has the datalayout string in it, so just
use that.

The logical flow here is kind of a mess. We were constructing
a temporary target machine in the asm parser to infer the datalayout,
throwing it away, and then creating another target machine for the
actual compilation. The flow of the Triple construction is still
convoluted, but we can at least drop the TargetMachine.
@arsenm arsenm added the tools:opt label Nov 26, 2025 — with Graphite App
Copy link
Contributor Author

arsenm commented Nov 26, 2025

@arsenm arsenm marked this pull request as ready for review November 26, 2025 00:47
Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I was quite unhappy with this approach when I initially made the change but at the time there was no better solution.

Fortunately #157612 cleaned this up! Thanks @rnk 👍


Triple TT(TripleStr);

std::string Str = TT.computeDataLayout();
Copy link
Member

@arichardson arichardson Nov 26, 2025

Choose a reason for hiding this comment

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

Can we also pass the value of the --target-abi= command line option here? Since depending on the ABI, there could be a different data layouts.
For testing this we could instantiate opt with e.g. MIPS n32 ABI or one of the ARM ABI variants that affect the data layout string.

I know this is a functional change rather than NFC, so also fine to defer until later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's also apparently a target-abi module flag, and this doesn't seem like it's all handled correctly

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it seems to be mostly ignored which causes a bunch of issues with LTO (especially on RISC-V) such as #50591 / #69780. Probably easiest to ignore that attribute for now and only look at command line flags.

@arsenm arsenm enabled auto-merge (squash) November 26, 2025 04:16
@arsenm arsenm merged commit e81a564 into main Nov 26, 2025
7 of 9 checks passed
@arsenm arsenm deleted the users/arsenm/opt/stop-creating-tm-compute-datalayout branch November 26, 2025 04:40
augusto2112 pushed a commit to augusto2112/llvm-project that referenced this pull request Dec 3, 2025
The Triple directly has the datalayout string in it, so just
use that.

The logical flow here is kind of a mess. We were constructing
a temporary target machine in the asm parser to infer the datalayout,
throwing it away, and then creating another target machine for the
actual compilation. The flow of the Triple construction is still
convoluted, but we can at least drop the TargetMachine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants