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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:558
+  if (!FirstLI->getAlignment())
+    return nullptr;
+  Align LoadAlignment(FirstLI->getAlignment());
----------------
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.


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77454