[PATCH] D41675: Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1)

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Much closer to reasonable, likely one or two more iterations.



================
Comment at: include/llvm/IR/IntrinsicInst.h:428
+      else {
+        // Manually inline body of getSourceAlignment().
+        // This class is the base class of MemTransferInst; invoking
----------------
Why is this not just a auto *this_MTI = cast<MemTransferInst>(this)?

I'm fine with this spelling, just point out the alternative.


================
Comment at: include/llvm/IR/IntrinsicInst.h:452
+      if (Intrinsic::memset != this->getIntrinsicID()) {
+        // Manually inline body of setSourceAlignment().
+        // This class is the base class of MemTransferInst; invoking
----------------
This ones a lot more awkward and the other idiom would be preferred.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:1613
       if (Align > MI->getAlignment())
-        MI->setAlignment(ConstantInt::get(MI->getAlignmentType(), Align));
+        MI->setAlignment(Align);
     }
----------------
You can and should land a refactoring without further review which does the following:
1) add a cover function setAlignment(unsigned) which internally creates the constant and passses it in
2) update the various callers (like this one) to use it

Doing so will remove this file entirely from the patch.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5013
     SDValue Op1 = getValue(I.getArgOperand(0));
     SDValue Op2 = getValue(I.getArgOperand(1));
     SDValue Op3 = getValue(I.getArgOperand(2));
----------------
Changes in this file (removing the raw attribute indexes) can be done on the existing API and landed without further review.  Please include the TODOs.


================
Comment at: lib/IR/AutoUpgrade.cpp:2401
                     "the default case");
   std::string Name = CI->getName();
   if (!Name.empty()) {
----------------
This file needs careful review.  I'll do it as a last resort, but if there's someone familiar with this code who wants to step in...

I'm deferring review on this file until everything else has settled.


================
Comment at: lib/IR/IRBuilder.cpp:81
 
-CallInst *IRBuilderBase::
-CreateMemSet(Value *Ptr, Value *Val, Value *Size, unsigned Align,
----------------
I think this is just whitespace?  If so, remove!


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:194
   if (CopyAlign < MinAlign) {
-    MI->setAlignment(ConstantInt::get(MI->getAlignmentType(), MinAlign, false));
+    MI->setAlignment(MinAlign);
     return MI;
----------------
As mentioned previously, use a cover function, separate and land.


================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1388
   SrcShadow = IRB.CreateBitCast(SrcShadow, Int8Ptr);
-  IRB.CreateCall(I.getCalledValue(), {DestShadow, SrcShadow, LenShadow,
-                                      AlignShadow, I.getVolatileCst()});
+  auto *MTI = cast<MemTransferInst>(
+      IRB.CreateCall(I.getCalledValue(),
----------------
Again, cover function.


================
Comment at: lib/Transforms/Scalar/AlignmentFromAssumptions.cpp:387
 
-        MI->setAlignment(ConstantInt::get(Type::getInt32Ty(
-          MI->getParent()->getContext()), NewDestAlignment));
+        MI->setAlignment(NewDestAlignment);
         ++NumMemIntAlignChanged;
----------------
Again, cover function.


================
Comment at: lib/Transforms/Scalar/SROA.cpp:2809
       if (II.getAlignment() > SliceAlign) {
-        Type *CstTy = II.getAlignmentCst()->getType();
-        II.setAlignment(
-            ConstantInt::get(CstTy, MinAlign(II.getAlignment(), SliceAlign)));
+        II.setAlignment(MinAlign(II.getAlignment(), SliceAlign));
       }
----------------
Cover function.


https://reviews.llvm.org/D41675