[PATCH] D77454: LoadInst should store Align, not MaybeAlign.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Looks much better, two comments below.



================
Comment at: llvm/include/llvm/IR/Instructions.h:184
-  LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr = "",
-           Instruction *InsertBefore = nullptr);
   LoadInst(Type *Ty, Value *Ptr, const Twine &NameStr, BasicBlock *InsertAtEnd);
----------------
efriedma wrote:
> jdoerfert wrote:
> > Why do we want to remove the default values?
> The four constructors that don't explicitly specify an alignment now grab the DataLayout from the last argument.  So it can't be null.
Makes sense. TBH, I would make it a reference so it breaks at compile time not runtime when null is passed explicitly.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:558
+  if (!FirstLI->getAlignment())
+    return nullptr;
+  Align LoadAlignment(FirstLI->getAlignment());
----------------
efriedma wrote:
> jdoerfert wrote:
> > On first glance I'm not sure if why the below algorithm would not work if the alignment of all loads is "false". 
> It would, but the logic is more complicated.
> 
> I just simplified this to assume loads have alignment set.
thx!


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:2572
       LoadInst *NewLI = IRB.CreateAlignedLoad(
-          TargetTy, getNewAllocaSlicePtr(IRB, LTy), getSliceAlign(TargetTy),
           LI.isVolatile(), LI.getName());
----------------
efriedma wrote:
> jdoerfert wrote:
> > I don't understand the change here and above tbh.
> 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.
Can you create a patch to remove this logic?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77454/new/

https://reviews.llvm.org/D77454