[PATCH] D63215: Fixing @llvm.memcpy not honoring volatile
JF Bastien via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 17 10:30:00 PDT 2019
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
More information about the llvm-commits
mailing list