[PATCH] D41675: Change memcpy/memove/memset to have dest and source alignment attributes.

[Philip Reames via Phabricator via llvm-commits]

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

Meta comment: This change is too large *conceptually*.  You need to break it apart in a way that the test changes are "obviously correct" because there's no possible change in behaviour.  At the moment, it a behaviour change might be lurking in the test changes and that'd never be seen.

My suggest approach would be to do this in multiple phases:

1. separate out the Attribute removal changes, unit test via C++
2. change how the alignment is represented (separate argument to param attributes), but add a temporary verifier rule that the source and dest have the same alignment.  Update the set functions to update both, and the read to take one of the at random (because they're equal).  No client code should change!  But this will include all of your *NFC* test updates.
3. Remove the temporary verifier rule and split the alignment API as appropriate.  Should have functional tests for refined behaviour (i.e. able to strengthen only one param). Likely can generalize some existing handling to any align param attribute.
4. Land any NFC renames you desire (i.e. memset Align to DestAlign).  These should not be intermixed with preceding changes!





================
Comment at: docs/LangRef.rst:10366
 
-If the call to this intrinsic has an alignment value that is not 0 or 1,
-then the caller guarantees that both the source and destination pointers
-are aligned to that boundary.
+The :ref:`align <_attr_align>` parameter attribute can be provided
+for the first and second arguments.
----------------
I might add a bit about the expected performance impact of adding align.


================
Comment at: docs/LangRef.rst:10382
 
+History
+"""""""
----------------
You can drop this.  The same information can be found by using the older docs directly.


================
Comment at: docs/LangRef.rst:10456
 
+History
+"""""""
----------------
Same


================
Comment at: docs/LangRef.rst:10525
+
+History
+"""""""
----------------
Same.


================
Comment at: include/llvm/IR/IRBuilder.h:424
+  CallInst *CreateMemCpy(Value *Dst, unsigned DstAlign, Value *Src,
+                         unsigned SrcAlign, uint64_t Size,
                          bool isVolatile = false, MDNode *TBAATag = nullptr,
----------------
To split the change, you could leave the Align parameter in place, and set both to the alignment.  Anyone who wanted to split them could directly access the parameter attributes on the resulting call.


================
Comment at: include/llvm/IR/IntrinsicInst.h:400
   public:
-    ConstantInt *getAlignmentCst() const {
-      return cast<ConstantInt>(const_cast<Value *>(getArgOperand(ARG_ALIGN)));
----------------
You could break up the patch by leaving these in place as cover functions.  You could do the following:
1) setAlignment sets alignment on both params
2) getAlignment returns the minimum of the two


================
Comment at: include/llvm/IR/IntrinsicInst.h:472
   class MemTransferInst : public MemIntrinsic {
+  private:
+    enum { ARG_SOURCE = 1 };
----------------
You can and should separate and land this NFC change.


================
Comment at: lib/CodeGen/SafeStack.cpp:567
+    // one for the destination of the memcpy?
+    IRB.CreateMemCpy(Off, Align, Arg, Arg->getParamAlignment(), Size);
   }
----------------
It's not obvious Align is correct here.  Please match the old behaviour and improve in a followon patch.  (i.e. pass the arg alignment to both)


================
Comment at: lib/CodeGen/SafeStackLayout.cpp:45
   StackObjects.push_back({V, Size, Alignment, Range});
+  ObjectAlignments[V] = Alignment;
   MaxAlignment = std::max(MaxAlignment, Alignment);
----------------
Isn't this already available in the other structure?


================
Comment at: lib/IR/Attributes.cpp:546
   if (!hasAttribute(Kind)) return *this;
-  AttrBuilder B;
-  B.addAttribute(Kind);
----------------
Huh?  Is this a separate bug fix?  If so, separate.  Otherwise, why is it in this patch?


https://reviews.llvm.org/D41675