[PATCH] D134535: Propagate Dst and Src alignment for mem{cpy|move}

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

What about keeping the changes completely mechanical and semantically the same for this large patch and fixing the issues separately ?



================
Comment at: llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp:98
     return EmitMOPS(AArch64ISD::MOPS_MEMSET, DAG, dl, Chain, Dst, Src, Size,
-                    Alignment, isVolatile, DstPtrInfo, MachinePointerInfo{});
+                    Alignment, Align(1), isVolatile, DstPtrInfo,
+                    MachinePointerInfo{});
----------------
Why this change ? I can see that this does not read, but what about keeping this change completely mechanical ?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:3268
+                          /*DstAlign*/ Outs[i].Flags.getNonZeroByValAlign(),
+                          /*SrcAlign*/ Outs[i].Flags.getNonZeroByValAlign(),
+                          /*isVol = */ false,
----------------
Looks like this might have been incorrect. Maybe add the author to that patch as an FYI ?


================
Comment at: llvm/lib/Target/ARM/ARMSelectionDAGInfo.cpp:188
   // This requires 4-byte alignment.
-  if (Alignment < Align(4))
+  if (DstAlign < Align(4) || SrcAlign < Align(4))
     return SDValue();
----------------
this used to be: `if (max(DstAlign, SrcAlign) < 4)`, ie.g. `if (DstAlign < Align(4) && SrcAlign < Align(4))`




================
Comment at: llvm/lib/Target/BPF/BPFSelectionDAGInfo.cpp:31
   unsigned CopyLen = ConstantSize->getZExtValue();
-  unsigned StoresNumEstimate = alignTo(CopyLen, Alignment) >> Log2(Alignment);
+  unsigned StoresNumEstimate = alignTo(CopyLen, DstAlign) >> Log2(DstAlign);
   // Impose the same copy length limit as MaxStoresPerMemcpy.
----------------
This is not NFC.


================
Comment at: llvm/lib/Target/Hexagon/HexagonSelectionDAGInfo.cpp:25
   ConstantSDNode *ConstantSize = dyn_cast<ConstantSDNode>(Size);
-  if (AlwaysInline || Alignment < Align(4) || !ConstantSize)
+  if (AlwaysInline || DstAlign < Align(4) || SrcAlign < Align(4) ||
+      !ConstantSize)
----------------
ditto


================
Comment at: llvm/lib/Target/X86/X86SelectionDAGInfo.cpp:186
 static MVT getOptimalRepmovsType(const X86Subtarget &Subtarget,
-                                 uint64_t Align) {
-  assert((Align != 0) && "Align is normalized");
-  assert(isPowerOf2_64(Align) && "Align is a power of 2");
-  switch (Align) {
-  case 1:
+                                 Align Alignment) {
+  if (Alignment == Align(1))
----------------
Can you do that in a separate patch ?


================
Comment at: llvm/lib/Target/XCore/XCoreISelLowering.cpp:1797
                               DAG.getConstant(StoreBits / 8, dl, MVT::i32),
-                              Alignment, false, isTail,
+                              ST->getAlign(), LD->getAlign(), false, isTail,
                               ST->getPointerInfo(), LD->getPointerInfo());
----------------
This might have been a bug, but again, let's fix it separately.


================
Comment at: llvm/lib/Target/XCore/XCoreSelectionDAGInfo.cpp:25
   // Call __memcpy_4 if the src, dst and size are all 4 byte aligned.
-  if (!AlwaysInline && Alignment >= Align(4) &&
+  if (!AlwaysInline && DstAlign >= Align(4) && SrcAlign >= Align(4) &&
       DAG.MaskedValueIsZero(Size, APInt(SizeBitWidth, 3))) {
----------------
this should be `( || )`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134535