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

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked 2 inline comments as done.
efriedma added inline comments.


================
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);
----------------
jdoerfert wrote:
> 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.
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.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:2572
       LoadInst *NewLI = IRB.CreateAlignedLoad(
-          TargetTy, getNewAllocaSlicePtr(IRB, LTy), getSliceAlign(TargetTy),
           LI.isVolatile(), LI.getName());
----------------
jdoerfert wrote:
> 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?
Split off the SROA changes into D78403.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77454