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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked 3 inline comments as done.
gchatelet added a comment.

In D63215#1540160 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5767
   if (Src.isUndef())
     return Chain;
 
----------------
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?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:5976
+          false /*AllowOverlap*/, DstPtrInfo.getAddrSpace(),
+          SrcPtrInfo.getAddrSpace(), MF.getFunction().getAttributes()))
     return SDValue();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63215