Page MenuHomePhabricator

LoadInst should store Align, not MaybeAlign.
ClosedPublic

Authored by efriedma on Apr 3 2020, 11:07 PM.

Details

Summary

The fact that loads and stores can have the alignment missing is a constant source of confusion: code that usually works can break down in rare cases. So fix the LoadInst API so the alignment is never missing.

To reduce the number of changes required to make this work, IRBuilder and certain LoadInst constructors will grab the module's datalayout and compute the alignment automatically. This is the same alignment instcombine would eventually apply anyway; we're just doing it earlier. There's a minor risk that the way we're retrieving the datalayout could break out-of-tree code, but I don't think that's likely.

This is the last in a series of patches, so most of the necessary changes have already been merged.

Diff Detail

Unit TestsFailed

TimeTest
870 msClang.CodeGen::temporary-lifetime.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/temporary-lifetime.cpp -std=c++11 -O1 -DWITH_DTOR -triple x86_64 -emit-llvm -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck -check-prefix=CHECK-DTOR /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/temporary-lifetime.cpp
910 msClang.CodeGenCXX::2003-11-29-DuplicatedCleanupTest.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -emit-llvm /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/2003-11-29-DuplicatedCleanupTest.cpp -o -
840 msClang.CodeGenCXX::condition.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc -triple x86_64-apple-darwin10 -emit-llvm -o - /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/condition.cpp | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/condition.cpp
850 msClang.CodeGenCXX::destructors.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/destructors.cpp -triple x86_64-apple-darwin10 -emit-llvm -o - -mconstructor-aliases -fcxx-exceptions -fexceptions -O1 -disable-llvm-passes -std=c++03 > /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/test/CodeGenCXX/Output/destructors.cpp.tmp
1,050 msClang.CodeGenCXX::goto.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/clang/11.0.0/include -nostdsysteminc /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/goto.cpp -triple=x86_64-apple-darwin10 -fcxx-exceptions -fexceptions -emit-llvm -std=c++98 -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck -check-prefix=CHECK -check-prefix=CHECK98 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGenCXX/goto.cpp
View Full Test Results (94 Failed)

Event Timeline

efriedma created this revision.Apr 3 2020, 11:07 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I'm in favor. The issues that arise due to confusion around the alignment are real.

I concur, this is in the same general direction as the troubles i was having with these in D76550.

arsenm added a subscriber: arsenm.Apr 4 2020, 12:20 PM

I don't think there should even be a MaybeAlign. There should always be an alignment in all relevant contexts

Thx a lot @efriedma!

The whole purpose of the effort behind Align/MaybeAlign was indeed to remove confusion (and bugs).
LLVM code is huge and I had to be conservative most of the time, navigating visually and trying to guess what was expected by interfaces. This is great to know that the LoadInst API needs a resolved alignment, definitely helps a lot!

I'm happy if we end up dropping MaybeAlign completely although I believe it's still necessary for some interfaces. The less the better obviously.

llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp
123

Is this necessary? LI->getAlign() should suffice.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
1027

auto Alignment = LI.getAlign();

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
559

Align LoadAlignment = FirstLI->getAlignment();

587

LoadAlignment = std::min(LoadAlignment, LI->getAlign());

courbet added inline comments.Apr 6 2020, 12:30 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
1380 ↗(On Diff #255005)

Is it always the case that the required alignment is the size of the type ? Shouldn't we ask the DataLayout ?

Do you want me to take over and implement the change @efriedma?

efriedma marked 4 inline comments as done.Apr 6 2020, 11:18 AM

If you want to take this over, sure, go for it. :)

llvm/include/llvm/IR/IRBuilder.h
1716 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

1731 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

1747 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/include/llvm/IR/Instructions.h
200

(Note this is currently based on D76269; master has some extra constructors. I'll try to get that merged soon.)

llvm/lib/AsmParser/LLParser.cpp
7004 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/lib/Bitcode/Reader/BitcodeReader.cpp
4830 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

4871 ↗(On Diff #255005)

This should use getValueOrABITypeAlignment

llvm/lib/CodeGen/AtomicExpandPass.cpp
1380 ↗(On Diff #255005)

Currently atomicrmw assumes the pointer is naturally aligned, i.e. has alignment equal to the alignment of the type.

If we add explicit alignment to atomicrmw at some point, we would use that here.

The ABI alignment isn't relevant here.

llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp
123

If you commit it separately, just "LI->getAlign()" will fail to build. If you commit it together, sure, it's redundant.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
587

Again, if you commit it separately, just "LI->getAlign()" will fail to build. If you commit it together, sure, it's redundant.

If you want to take this over, sure, go for it. :)

I wanted to but unfortunately I've to take that back. I'll be AFK for several weeks :-/

efriedma updated this revision to Diff 255564.Apr 6 2020, 6:47 PM

Rebased. I landed a bunch of the trivial non-functional changes.

Sorry for the wait. Some comments, mostly nits, some questions.

llvm/include/llvm/IR/IRBuilder.h
1747 ↗(On Diff #255005)

Why doesn't it ?

llvm/include/llvm/IR/Instructions.h
184

Why do we want to remove the default values?

llvm/lib/IR/Instructions.cpp
1341

Nit: getModule

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2314–2315

Unrelated

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
1027

I'm against auto here

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
558

On first glance I'm not sure if why the below algorithm would not work if the alignment of all loads is "false".

llvm/lib/Transforms/Scalar/SROA.cpp
2572

I don't understand the change here and above tbh.

llvm/lib/Transforms/Utils/CodeExtractor.cpp
1174 ↗(On Diff #255564)

Unrelated, just commit his.

efriedma updated this revision to Diff 259385.Apr 22 2020, 2:00 PM
efriedma marked 2 inline comments as done.
efriedma retitled this revision from [WIP] LoadInst should store Align, not MaybeAlign. to LoadInst should store Align, not MaybeAlign..
efriedma edited the summary of this revision. (Show Details)

Rebase. Address review comments.

This is based on D78403. Regression tests should pass now.

efriedma added inline comments.Apr 22 2020, 2:05 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
558

It would, but the logic is more complicated.

I just simplified this to assume loads have alignment set.

llvm/lib/Transforms/Scalar/SROA.cpp
2572

getSliceAlign() has code to try to avoid explicitly setting the alignment on load and store instructions, if it would be the same as the ABI alignment of the operation. This is complete nonsense, of course; it's actually a no-op on trunk because CreateAlignedLoad will recompute exactly the same alignment anyway.

efriedma marked an inline comment as done.Apr 22 2020, 2:10 PM
efriedma added inline comments.
llvm/include/llvm/IR/Instructions.h
184

The four constructors that don't explicitly specify an alignment now grab the DataLayout from the last argument. So it can't be null.

Looks much better, two comments below.

llvm/include/llvm/IR/Instructions.h
184

Makes sense. TBH, I would make it a reference so it breaks at compile time not runtime when null is passed explicitly.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
558

thx!

llvm/lib/Transforms/Scalar/SROA.cpp
2572

Can you create a patch to remove this logic?

efriedma marked 2 inline comments as done.
efriedma added inline comments.
llvm/include/llvm/IR/Instructions.h
184

Maybe it should be a reference... but I I don't really want to sign up to change every constructor for every Instruction to pass a reference instead of a pointer.

llvm/lib/Transforms/Scalar/SROA.cpp
2572

Split off the SROA changes into D78403.

I'll accept tomorrow if no one raises a concern.

llvm/include/llvm/IR/Instructions.h
184

Yeah... Interesting that no in-tree user passes a nullptr. Maybe people don't do that.

Is this patch still supposed to land? If so can you format and rebase?

llvm/lib/Transforms/Scalar/SROA.cpp
1278

rename Align to Alignment and get rid of llvm::

jdoerfert accepted this revision.May 13 2020, 8:36 AM

Forgot about this. LGTM, after the comments are addressed.

This revision is now accepted and ready to land.May 13 2020, 8:36 AM
efriedma updated this revision to Diff 263835.May 13 2020, 1:04 PM

Rebased, fixed clang-format.

This revision was automatically updated to reflect the committed changes.