Fixing @llvm.memcpy not honoring volatile

[gchatelet]

Summary

https://bugs.llvm.org/show_bug.cgi?id=42254

[jfb]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

5774

Seems like we don't want to nop stuff if volatile. Can you add a FIXME?

5804

Names like this are usually written as /*IsMemset=*/false. clang-tidy recognizes and checks that style. Can you update to follow this style here and otherwhere in the patch?

5960

Ditto on nop'ing volatile.

5983

Seems you want !isVol for AllowOverlap here as well.

6068

Ditto.

[efriedma]

What happens if findOptimalMemOpLowering fails? We have other ways of lowering memcpy/memset that could also violate the "one store per byte" rule.

[gchatelet]

In D63215#1540160, @efriedma wrote:

What happens if findOptimalMemOpLowering fails? We have other ways of lowering memcpy/memset that could also violate the "one store per byte" rule.

Yes indeed, I need to take care of this as well.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

5774

I'm not familiar with the semantic of Undef here. The documentation says that the SDNode type is undefined but I fail to understand what it means for a volatile copy. Can you explain it to me?

5983

So findOptimalMemOpLowering does not work for memmove when AllowOverlap==true, it generates the correct code for volatile==true (aka AllowOverlap==false) but not for volatile==false:

	.globl	move_7_bytes
	.p2align	4, 0x90
	.type	move_7_bytes,@function
move_7_bytes:
	.cfi_startproc
# %bb.0:
	movq	(%rsi), %rax
	movq	%rax, (%rdi)
	retq
.Lfunc_end2:
	.size	move_7_bytes, .Lfunc_end2-move_7_bytes
	.cfi_endproc

It definitely looks like a bug but I'm reluctant to touch this code - too complex and not unit tested.

Let me know what you think.

[gchatelet]

In D63215#1541086, @gchatelet wrote:

In D63215#1540160, @efriedma wrote:

What happens if findOptimalMemOpLowering fails? We have other ways of lowering memcpy/memset that could also violate the "one store per byte" rule.

Yes indeed, I need to take care of this as well.

So I've looked at the code, I believe the general case is covered.
What's left is the call to libc's memcpy (which is OK until the standard is updated) and the target specific code: nothing pops out particularly here but I'm not really qualified to review all of it.

BTW I'm not sure Intel's rep movs gives any guarantee on the overlapping access, it's microcoded nowadays.

[gchatelet]

• Update argument comments

[jfb]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

5774

I think this can be fixed in a later patch: I see volatile as something the compiler isn't allowed to remove from code. Doesn't matter if we're copying something invalid: the programmer asked for something silly, the Standard tells us to honor the silly, so we should.

5983

Leave a FIXME explaining this bug.

[gchatelet]

• Address comments

[jfb]

Looks good overall, but I'd like to get someone else's input as well.

[gchatelet]

In D63215#1555894, @jfb wrote:

Looks good overall, but I'd like to get someone else's input as well.

@efriedma ? @t.p.northover ?

[efriedma]

Maybe mention in the commit message that this is explicitly not addressing target-specific code, or calls to memcpy?

I don't agree with the "undef" FIXME (a load/store from undef is UB, whether or not it's volatile), but the check for undef is very unlikely to matter in practice anyway.

Otherwise LGTM.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 
 static SDValue getMemcpyLoadsAndStores(SelectionDAG &DAG, const SDLoc &dl,
                                        SDValue Chain, SDValue Dst, SDValue Src,
                                        uint64_t Size, unsigned Align,
                                        bool isVol, bool AlwaysInline,
                                        MachinePointerInfo DstPtrInfo,
                                        MachinePointerInfo SrcPtrInfo) {
   // Turn a memcpy of undef to nop.
+  // FIXME: We need to honor volatile even is Src is undef.
   if (Src.isUndef())
     return Chain;
 
   // Expand memcpy to a series of load and store ops if the size operand falls
   // below a certain threshold.
   // TODO: In the AlwaysInline case, if the size is big then generate a loop
   // rather than maybe a humongous number of loads and stores.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   const DataLayout &DL = DAG.getDataLayout();
   LLVMContext &C = *DAG.getContext();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI.isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   unsigned SrcAlign = DAG.InferPtrAlignment(Src);
   if (Align > SrcAlign)
     SrcAlign = Align;
   ConstantDataArraySlice Slice;
   bool CopyFromConstant = isMemSrcFromConstant(Src, Slice);
   bool isZeroConstant = CopyFromConstant && Slice.Array == nullptr;
   unsigned Limit = AlwaysInline ? ~0U : TLI.getMaxStoresPerMemcpy(OptSize);
 
-  if (!TLI.findOptimalMemOpLowering(MemOps, Limit, Size,
-                                    (DstAlignCanChange ? 0 : Align),
-                                    (isZeroConstant ? 0 : SrcAlign),
-                                    false, false, CopyFromConstant, true,
-                                    DstPtrInfo.getAddrSpace(),
-                                    SrcPtrInfo.getAddrSpace(),
-                                    MF.getFunction().getAttributes()))
+  if (!TLI.findOptimalMemOpLowering(
+          MemOps, Limit, Size, (DstAlignCanChange ? 0 : Align),
+          (isZeroConstant ? 0 : SrcAlign), /*IsMemset=*/false,
+          /*ZeroMemset=*/false, /*MemcpyStrSrc=*/CopyFromConstant,
+          /*AllowOverlap=*/!isVol, DstPtrInfo.getAddrSpace(),
+          SrcPtrInfo.getAddrSpace(), MF.getFunction().getAttributes()))
     return SDValue();
 
   if (DstAlignCanChange) {
     Type *Ty = MemOps[0].getTypeForEVT(C);
     unsigned NewAlign = (unsigned)DL.getABITypeAlignment(Ty);
 
     // Don't promote to an alignment that would require dynamic stack
     // realignment.

 
 static SDValue getMemmoveLoadsAndStores(SelectionDAG &DAG, const SDLoc &dl,
                                         SDValue Chain, SDValue Dst, SDValue Src,
                                         uint64_t Size, unsigned Align,
                                         bool isVol, bool AlwaysInline,
                                         MachinePointerInfo DstPtrInfo,
                                         MachinePointerInfo SrcPtrInfo) {
   // Turn a memmove of undef to nop.
+  // FIXME: We need to honor volatile even is Src is undef.
   if (Src.isUndef())
     return Chain;
 
   // Expand memmove to a series of load and store ops if the size operand falls
   // below a certain threshold.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   const DataLayout &DL = DAG.getDataLayout();
   LLVMContext &C = *DAG.getContext();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI.isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   unsigned SrcAlign = DAG.InferPtrAlignment(Src);
   if (Align > SrcAlign)
     SrcAlign = Align;
   unsigned Limit = AlwaysInline ? ~0U : TLI.getMaxStoresPerMemmove(OptSize);
-
-  if (!TLI.findOptimalMemOpLowering(MemOps, Limit, Size,
-                                    (DstAlignCanChange ? 0 : Align), SrcAlign,
-                                    false, false, false, false,
-                                    DstPtrInfo.getAddrSpace(),
-                                    SrcPtrInfo.getAddrSpace(),
-                                    MF.getFunction().getAttributes()))
+  // FIXME: `AllowOverlap` should really be `!isVol` but there is a bug in
+  // findOptimalMemOpLowering. Meanwhile, setting it to `false` produces the
+  // correct code.
+  bool AllowOverlap = false;
+  if (!TLI.findOptimalMemOpLowering(
+          MemOps, Limit, Size, (DstAlignCanChange ? 0 : Align), SrcAlign,
+          /*IsMemset=*/false, /*ZeroMemset=*/false, /*MemcpyStrSrc=*/false,
+          AllowOverlap, DstPtrInfo.getAddrSpace(), SrcPtrInfo.getAddrSpace(),
     return SDValue();
 
   if (DstAlignCanChange) {
     Type *Ty = MemOps[0].getTypeForEVT(C);
     unsigned NewAlign = (unsigned)DL.getABITypeAlignment(Ty);
     if (NewAlign > Align) {
       // Give the stack frame object a larger alignment if needed.
       if (MFI.getObjectAlignment(FI->getIndex()) < NewAlign)

 /// The function tries to replace 'llvm.memset' intrinsic with several store
 /// operations and value calculation code. This is usually profitable for small
 /// memory size.
 static SDValue getMemsetStores(SelectionDAG &DAG, const SDLoc &dl,
                                SDValue Chain, SDValue Dst, SDValue Src,
                                uint64_t Size, unsigned Align, bool isVol,
                                MachinePointerInfo DstPtrInfo) {
   // Turn a memset of undef to nop.
+  // FIXME: We need to honor volatile even is Src is undef.
   if (Src.isUndef())
     return Chain;
 
   // Expand memset to a series of load/store ops if the size operand
   // falls below a certain threshold.
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   std::vector<EVT> MemOps;
   bool DstAlignCanChange = false;
   MachineFunction &MF = DAG.getMachineFunction();
   MachineFrameInfo &MFI = MF.getFrameInfo();
   bool OptSize = shouldLowerMemFuncForSize(MF);
   FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Dst);
   if (FI && !MFI.isFixedObjectIndex(FI->getIndex()))
     DstAlignCanChange = true;
   bool IsZeroVal =
     isa<ConstantSDNode>(Src) && cast<ConstantSDNode>(Src)->isNullValue();
-  if (!TLI.findOptimalMemOpLowering(MemOps, TLI.getMaxStoresPerMemset(OptSize),
-                                    Size, (DstAlignCanChange ? 0 : Align), 0,
-                                    true, IsZeroVal, false, true,
-                                    DstPtrInfo.getAddrSpace(), ~0u,
-                                    MF.getFunction().getAttributes()))
+  if (!TLI.findOptimalMemOpLowering(
+          MemOps, TLI.getMaxStoresPerMemset(OptSize), Size,
+          (DstAlignCanChange ? 0 : Align), 0, /*IsMemset=*/true,
+          /*ZeroMemset=*/IsZeroVal, /*MemcpyStrSrc=*/false,
+          /*AllowOverlap=*/!isVol, DstPtrInfo.getAddrSpace(), ~0u,
     return SDValue();
 
   if (DstAlignCanChange) {
     Type *Ty = MemOps[0].getTypeForEVT(*DAG.getContext());
     unsigned NewAlign = (unsigned)DAG.getDataLayout().getABITypeAlignment(Ty);
     if (NewAlign > Align) {
       // Give the stack frame object a larger alignment if needed.
       if (MFI.getObjectAlignment(FI->getIndex()) < NewAlign)

llvm/test/CodeGen/X86/volatile-memstores-nooverlapping-load-stores.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1 immarg) #1
+define dso_local void @copy_7_bytes(i8* noalias nocapture, i8* noalias nocapture readonly) nounwind #0 {
+; CHECK-LABEL: copy_7_bytes:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl (%rsi), %eax
+; CHECK-NEXT:    movl 3(%rsi), %ecx
+; CHECK-NEXT:    movl %ecx, 3(%rdi)
+; CHECK-NEXT:    movl %eax, (%rdi)
+; CHECK-NEXT:    retq
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %1, i64 7, i1 false)
+  ret void
+}
+define dso_local void @copy_7_bytes_volatile(i8* noalias nocapture, i8* noalias nocapture readonly) nounwind #0 {
+; CHECK-LABEL: copy_7_bytes_volatile:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movb 6(%rsi), %al
+; CHECK-NEXT:    movb %al, 6(%rdi)
+; CHECK-NEXT:    movzwl 4(%rsi), %eax
+; CHECK-NEXT:    movw %ax, 4(%rdi)
+; CHECK-NEXT:    movl (%rsi), %eax
+; CHECK-NEXT:    movl %eax, (%rdi)
+; CHECK-NEXT:    retq
+  tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %1, i64 7, i1 true)
+  ret void
+}
+
+
+declare void @llvm.memmove.p0i8.p0i8.i64(i8* nocapture, i8* nocapture readonly, i64, i1 immarg) #1
+define dso_local void @move_7_bytes(i8* nocapture, i8* nocapture readonly) nounwind #0 {
+; CHECK-LABEL: move_7_bytes:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl (%rsi), %eax
+; CHECK-NEXT:    movzwl 4(%rsi), %ecx
+; CHECK-NEXT:    movb 6(%rsi), %dl
+; CHECK-NEXT:    movb %dl, 6(%rdi)
+; CHECK-NEXT:    movw %cx, 4(%rdi)
+; CHECK-NEXT:    movl %eax, (%rdi)
+; CHECK-NEXT:    retq
+  tail call void @llvm.memmove.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %1, i64 7, i1 false)
+  ret void
+}
+define dso_local void @move_7_bytes_volatile(i8* nocapture, i8* nocapture readonly) nounwind #0 {
+; CHECK-LABEL: move_7_bytes_volatile:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl (%rsi), %eax
+; CHECK-NEXT:    movzwl 4(%rsi), %ecx
+; CHECK-NEXT:    movb 6(%rsi), %dl
+; CHECK-NEXT:    movb %dl, 6(%rdi)
+; CHECK-NEXT:    movw %cx, 4(%rdi)
+; CHECK-NEXT:    movl %eax, (%rdi)
+; CHECK-NEXT:    retq
+  tail call void @llvm.memmove.p0i8.p0i8.i64(i8* align 1 %0, i8* align 1 %1, i64 7, i1 true)
+  ret void
+}
+
+
+declare void @llvm.memset.p0i8.i64(i8* nocapture writeonly, i8 , i64, i1 immarg) #1
+define dso_local void @set_7_bytes(i8* noalias nocapture) nounwind #0 {
+; CHECK-LABEL: set_7_bytes:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl $16843009, 3(%rdi) # imm = 0x1010101
+; CHECK-NEXT:    movl $16843009, (%rdi) # imm = 0x1010101
+; CHECK-NEXT:    retq
+  tail call void @llvm.memset.p0i8.i64(i8* align 1 %0, i8 1, i64 7, i1 false)
+  ret void
+}
+define dso_local void @set_7_bytes_volatile(i8* noalias nocapture) nounwind #0 {
+; CHECK-LABEL: set_7_bytes_volatile:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movb $1, 6(%rdi)
+; CHECK-NEXT:    movw $257, 4(%rdi) # imm = 0x101
+; CHECK-NEXT:    movl $16843009, (%rdi) # imm = 0x1010101
+; CHECK-NEXT:    retq
+  tail call void @llvm.memset.p0i8.i64(i8* align 1 %0, i8 1, i64 7, i1 true)
+  ret void
+}
+
+attributes #0 = { noreturn nounwind uwtable "target-cpu"="x86-64" }
+attributes #1 = { argmemonly nounwind }