[PATCH] D86883: [X86] Add support for using fast short rep mov for memcpy lowering.

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3112
+  assert(Src.getValueType() == Dst.getValueType());
+  SDValue SizeNode = DAG.getConstant(Flags.getByValSize(), dl, Src.getValueType());
 
----------------
craig.topper wrote:
> Maybe just use getIntPtrConstant instead of getConstant?
My bad. This should actually be an int const rather than an int pointer const. Will change it to check if it's 64 bit instead.


================
Comment at: llvm/lib/Target/X86/X86SelectionDAGInfo.cpp:198
   SDValue InFlag;
-  Chain = DAG.getCopyToReg(Chain, dl, CX, Size, InFlag);
+  Chain = DAG.getCopyToReg(
+      Chain, dl, CX,
----------------
yamauchi wrote:
> craig.topper wrote:
> > yamauchi wrote:
> > > craig.topper wrote:
> > > > yamauchi wrote:
> > > > > craig.topper wrote:
> > > > > > Is this needed because we're no longer calling this with just constants we fixed the size of?
> > > > > Do you mean, "is the getCopyToReg call needed"? The size needs to be loaded into the RCX/ECX register for the rep movs instruction and the callers aren't doing that. So, it seems yes. Did you mean in a different way?
> > > > I was asking why the getZExtOrTrunc was added. do we create memcpy's where the size type isn't the same size as the pointer?
> > > Without the getZExtOrTrunc, I get this fatal error, for example,
> > > 
> > > Cannot copy EAX to RCX
> > > fatal error: error in backend: Cannot emit physreg copy instruction
> > > 
> > > which is coming from X86InstrInfo::copyPhysReg.
> > > 
> > > Re: "do we create memcpy's where the size type isn't the same size as the pointer?" yes, it seems so. 
> > Does that occur with the included test case or some other testing?
> The type mismatch is coming from here:
> 
> ```
> static SDValue CreateCopyOfByValArgument(SDValue Src, SDValue Dst,
>                                          SDValue Chain, ISD::ArgFlagsTy Flags,
>                                          SelectionDAG &DAG, const SDLoc &dl) {
>   SDValue SizeNode = DAG.getConstant(Flags.getByValSize(), dl, MVT::i32);
>   ...
> ```
> 
> I'm going to fix that instead.
It occurs in a clang bootstrap build. Added a reduced test below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86883