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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

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.



================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp:117
+        /* CopyLen */ CopyLen,
+        /* SrcAlign */ MaybeAlign(LI->getAlign()).valueOrOne(),
+        /* DestAlign */ SI->getAlign().valueOrOne(),
----------------
Is this necessary? `LI->getAlign()` should suffice.


================
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(),
----------------
`auto Alignment = LI.getAlign();`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:559
+    return nullptr;
+  Align LoadAlignment(FirstLI->getAlignment());
   unsigned LoadAddrSpace = FirstLI->getPointerAddressSpace();
----------------
`Align LoadAlignment = FirstLI->getAlignment();`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:594
 
-    LoadAlignment = std::min(LoadAlignment, MaybeAlign(LI->getAlignment()));
+    LoadAlignment = std::min(LoadAlignment, Align(LI->getAlignment()));
 
----------------
`LoadAlignment = std::min(LoadAlignment, LI->getAlign());`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77454