[PATCH] D63215: Fixing @llvm.memcpy not honoring volatile

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5767
   if (Src.isUndef())
     return Chain;
 
----------------
gchatelet wrote:
> jfb wrote:
> > Seems like we don't want to nop stuff if volatile. Can you add a FIXME?
> 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?
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5976
+          false /*AllowOverlap*/, DstPtrInfo.getAddrSpace(),
+          SrcPtrInfo.getAddrSpace(), MF.getFunction().getAttributes()))
     return SDValue();
----------------
gchatelet wrote:
> jfb wrote:
> > Seems you want `!isVol` for `AllowOverlap` here as well.
> 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, at 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.
Leave a FIXME explaining this bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63215