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

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked 4 inline comments as done.
efriedma added a comment.

If you want to take this over, sure, go for it. :)



================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1716
+      const DataLayout &DL = BB->getParent()->getParent()->getDataLayout();
+      Align = DL.getABITypeAlign(Ty);
+    }
----------------
This should use getValueOrABITypeAlignment


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1731
+      const DataLayout &DL = BB->getParent()->getParent()->getDataLayout();
+      Align = DL.getABITypeAlign(Ty);
+    }    
----------------
This should use getValueOrABITypeAlignment


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


================
Comment at: llvm/include/llvm/IR/Instructions.h:200
+           Align Align, AtomicOrdering Order, SyncScope::ID SSID,
            BasicBlock *InsertAtEnd);
 
----------------
(Note this is currently based on D76269; master has some extra constructors.  I'll try to get that merged soon.)


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:7004
+
+  Inst = new LoadInst(Ty, Val, "", isVolatile, *Alignment, Ordering, SSID);
   return AteExtraComma ? InstExtraComma : InstNormal;
----------------
This should use getValueOrABITypeAlignment


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4830
+        Align = DL.getABITypeAlign(Ty);
+      }
+      I = new LoadInst(Ty, Op, "", Record[OpNum + 1], *Align);
----------------
This should use getValueOrABITypeAlignment


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4871
+        Align = DL.getABITypeAlign(Ty);
+      }      
+      I = new LoadInst(Ty, Op, "", Record[OpNum + 1], *Align, Ordering, SSID);
----------------
This should use getValueOrABITypeAlignment




================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:1380
   // Atomics require at least natural alignment.
-  InitLoaded->setAlignment(MaybeAlign(ResultTy->getPrimitiveSizeInBits() / 8));
+  InitLoaded->setAlignment(Align(ResultTy->getPrimitiveSizeInBits() / 8));
   Builder.CreateBr(LoopBB);
----------------
courbet wrote:
> Is it always the case that the required alignment is the size of the type ? Shouldn't we ask the DataLayout ?
Currently atomicrmw assumes the pointer is naturally aligned, i.e. has alignment equal to the alignment of the type.

If we add explicit alignment to atomicrmw at some point, we would use that here.

The ABI alignment isn't relevant here.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp:117
+        /* CopyLen */ CopyLen,
+        /* SrcAlign */ MaybeAlign(LI->getAlign()).valueOrOne(),
+        /* DestAlign */ SI->getAlign().valueOrOne(),
----------------
gchatelet wrote:
> Is this necessary? `LI->getAlign()` should suffice.
If you commit it separately, just "LI->getAlign()" will fail to build.  If you commit it together, sure, it's redundant.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:594
 
-    LoadAlignment = std::min(LoadAlignment, MaybeAlign(LI->getAlignment()));
+    LoadAlignment = std::min(LoadAlignment, Align(LI->getAlignment()));
 
----------------
gchatelet wrote:
> `LoadAlignment = std::min(LoadAlignment, LI->getAlign());`
Again, if you commit it separately, just "LI->getAlign()" will fail to build. If you commit it together, sure, it's redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77454