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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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



================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1747
+      const DataLayout &DL = BB->getParent()->getParent()->getDataLayout();
+      Align = DL.getABITypeAlign(Ty);
+    }    
----------------
efriedma wrote:
> This should use getValueOrABITypeAlignment
Why doesn't it ?


================
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);
----------------
Why do we want to remove the default values?


================
Comment at: llvm/lib/IR/Instructions.cpp:1341
+Align computeLoadAlign(Type *Ty, BasicBlock *BB) {
+  const DataLayout &DL = BB->getParent()->getParent()->getDataLayout();
+  return DL.getABITypeAlign(Ty);
----------------
Nit: `getModule`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2327
         ExtSrc0->getType() == ExtSrc1->getType()) {
-      Value *F = Intrinsic::getDeclaration(II->getModule(), II->getIntrinsicID(),
-                                           { ExtSrc0->getType() });
----------------
Unrelated


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1027
       // load (select (Cond, &V1, &V2))  --> select(Cond, load &V1, load &V2).
-      const MaybeAlign Alignment(LI.getAlignment());
+      auto Alignment(LI.getAlign());
       if (isSafeToLoadUnconditionally(SI->getOperand(1), LI.getType(),
----------------
gchatelet wrote:
> `auto Alignment = LI.getAlign();`
I'm against auto here


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:558
+  if (!FirstLI->getAlignment())
+    return nullptr;
+  Align LoadAlignment(FirstLI->getAlignment());
----------------
On first glance I'm not sure if why the below algorithm would not work if the alignment of all loads is "false". 


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:2572
       LoadInst *NewLI = IRB.CreateAlignedLoad(
-          TargetTy, getNewAllocaSlicePtr(IRB, LTy), getSliceAlign(TargetTy),
           LI.isVolatile(), LI.getName());
----------------
I don't understand the change here and above tbh.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1174
     Reloads.push_back(load);
-    codeReplacer->getInstList().push_back(load);
     std::vector<User *> Users(outputs[i]->user_begin(), outputs[i]->user_end());
----------------
Unrelated, just commit his.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77454