This is an archive of the discontinued LLVM Phabricator instance.

[opt] Infer DataLayout from triple if not specified
ClosedPublic

Authored by arichardson on Jan 5 2023, 7:26 AM.

Details

Summary

There are many tests that specify a target triple/CPU flags but no
DataLayout which can lead to IR being generated that has unusual
behaviour. This commit attempts to use the default DataLayout based
on the relevant flags if there is no explicit override on the command
line or in the IR file.

One thing that is not currently possible to differentiate from a missing
datalayout target datalayout = "" in the IR file since the current
APIs don't allow detecting this case. If it is considered useful to
support this case (instead of just using something basic like "e"),
I can change IR parsers to track whether they have seen such a
directive and change the callback type.

Diff Detail

Event Timeline

arichardson created this revision.Jan 5 2023, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 7:26 AM
arichardson published this revision for review.Jan 5 2023, 7:33 AM

Haven't updated all tests this affects yet, wanted to check if this is an acceptable change before spending more time on it.

nikic added a comment.Jan 6 2023, 1:26 AM

Sounds reasonable to me. In fact, I kind of thought that this is how it already works...

llvm/test/Transforms/InstCombine/fls-i16.ll
1

--force-update here and in a few more places.

llvm/test/Transforms/InstSimplify/ConstProp/calls-math-finite.ll
3

Please precommit a regeneration of this test -- not really clear what actually changed here (if anything).

Rebase and fix all tests

arichardson marked an inline comment as done.

Correctly regen sincospi.ll

Add a missing triple to no-libcalls.ll to ensure 8-byte pointer size

CC: @arsenm for non-obvious AMDGPU test diffs

llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-sincos.defined.nobuiltin.ll
59–63

This seems correct since it's now allocating in the alloca AS, but would be good to get a confirmation here.

llvm/test/CodeGen/AMDGPU/rewrite-out-arguments.ll
494

I'm not quite sure what is going on in this test, it seems like the correct datalayout is causing a huge difference.

llvm/test/CodeGen/BPF/CORE/field-reloc-bitfield-1.ll
50

Not quite sure what is going here would be good to get a second opinion.

llvm/test/Instrumentation/SanitizerCoverage/cmp-tracing-api-x86_32.ll
9

This is rather odd, sounds like we are introducing a unnecessary ptrtoint/inttoptr pair.

16

This looks correct to me but worth double-checking.

llvm/test/Transforms/AtomicExpand/AMDGPU/expand-atomic-i8.ll
8

i32 vs i64 change so I had to use a different prefix

llvm/test/Transforms/AtomicExpand/PowerPC/cmpxchg.ll
36–37

Looks like the correct data layout means we now call the optimized libcall here?

llvm/test/Transforms/CodeGenPrepare/AMDGPU/sink-addrspacecast.ll
128

I have no idea why this one is no longer being sunk.

152

Same here.

llvm/test/Transforms/InferFunctionAttrs/annotate.ll
5

Had to change this to ppc64 since the function signatures only work for 64-bit systems (with a 32-bit datalayout vec_calloc is not detected as a libcall).

llvm/test/Transforms/LoopStrengthReduce/X86/2012-01-13-phielim.ll
6

The diffs here look reasonable, but I'm not sure why they are happening.

llvm/test/Transforms/LoopVectorize/RISCV/interleaved-accesses.ll
80

This one might need further investigation.

llvm/test/Transforms/SafeStack/X86/setjmp2.ll
3

Had to split this into two due to changed alignment.

MaskRay accepted this revision.Oct 9 2023, 9:50 PM

This is definitely the correct thing to do. Thanks for fixing this... You likely need to rebase. For courtesy to experimental targets, consider -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD='ARC;AVR;CSKY;DirectX;LoongArch;M68k;SPIRV;Xtensa' and fix possible failures from the experimental targets.

This revision is now accepted and ready to land.Oct 9 2023, 9:50 PM
arichardson added a reviewer: Restricted Project.Oct 10 2023, 6:04 PM

This is definitely the correct thing to do. Thanks for fixing this... You likely need to rebase. For courtesy to experimental targets, consider -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD='ARC;AVR;CSKY;DirectX;LoongArch;M68k;SPIRV;Xtensa' and fix possible failures from the experimental targets.

Thanks for reviewing!
I originally tested with -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=all and didn't see any new failures (SPIRV was broken already when I tested). Will test again tomorrow.

I will wait for @arsenm (or someone else from the amdgpu team) to check the amdgpu behaviour changes before committing.

aeubanks accepted this revision.Oct 10 2023, 6:18 PM

This is definitely the correct thing to do. Thanks for fixing this... You likely need to rebase. For courtesy to experimental targets, consider -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD='ARC;AVR;CSKY;DirectX;LoongArch;M68k;SPIRV;Xtensa' and fix possible failures from the experimental targets.

Thanks for reviewing!
I originally tested with -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=all and didn't see any new failures (SPIRV was broken already when I tested). Will test again tomorrow.

I will wait for @arsenm (or someone else from the amdgpu team) to check the amdgpu behaviour changes before committing.

Ping @arsenm

arsenm added inline comments.Oct 25 2023, 3:03 AM
llvm/test/CodeGen/AMDGPU/amdgpu-simplify-libcall-sincos.defined.nobuiltin.ll
59–63

Yes

llvm/test/CodeGen/AMDGPU/rewrite-out-arguments.ll
494

This was expecting the pointer type to match the address space, so the transform didn't happen. these need to get all addrspace(5)s

llvm/test/Transforms/AtomicExpand/AMDGPU/expand-atomic-i8.ll
8

use multiple prefixes? Also seems wrong that r600 would still be using A0

arichardson marked 3 inline comments as done.

Rebased on top of https://github.com/llvm/llvm-project/pull/70269. Planning to commit this once that PR lands.

llvm/test/CodeGen/AMDGPU/rewrite-out-arguments.ll
494
llvm/test/Transforms/AtomicExpand/AMDGPU/expand-atomic-i8.ll
8

Updated to use GCN/R600 prefix.

lgtm with test nit

llvm/test/Transforms/AtomicExpand/AMDGPU/expand-atomic-i8.ll
2–4

I meant -check-prefixes=CHECK,GCN and -check-prefixes=CHECK,R600. Most of the functions should still be the same

This revision was landed with ongoing or failed builds.Oct 26 2023, 12:12 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Oct 26 2023, 2:10 PM

@arichardson, your commit is causing 9 failures on the PS4 linux bot (https://lab.llvm.org/buildbot/#/builders/139/builds/52287), and 10 failures on the PS5 Windows bot (https://lab.llvm.org/buildbot/#/builders/216/builds/29384). The extra Windows failure is because the test llvm/test/tools/opt/invalid-target.ll is not accounting for Windows executables, it is checing for opt not opt.exe which is what appears in the warning message on Windows.

Can you please take a look and revert if you need time to investigate?

@arichardson, your commit is causing 9 failures on the PS4 linux bot (https://lab.llvm.org/buildbot/#/builders/139/builds/52287), and 10 failures on the PS5 Windows bot (https://lab.llvm.org/buildbot/#/builders/216/builds/29384). The extra Windows failure is because the test llvm/test/tools/opt/invalid-target.ll is not accounting for Windows executables, it is checing for opt not opt.exe which is what appears in the warning message on Windows.

Can you please take a look and revert if you need time to investigate?

Those should hopefully be fixed now, and about to push the windows build fix.

dyung added a comment.Oct 26 2023, 2:41 PM

@arichardson, your commit is causing 9 failures on the PS4 linux bot (https://lab.llvm.org/buildbot/#/builders/139/builds/52287), and 10 failures on the PS5 Windows bot (https://lab.llvm.org/buildbot/#/builders/216/builds/29384). The extra Windows failure is because the test llvm/test/tools/opt/invalid-target.ll is not accounting for Windows executables, it is checing for opt not opt.exe which is what appears in the warning message on Windows.

Can you please take a look and revert if you need time to investigate?

Those should hopefully be fixed now, and about to push the windows build fix.

Thanks! Looks like you may need to update one additional test? https://lab.llvm.org/buildbot/#/builders/139/builds/52298