[PATCH] D129727: [ARM64EC 11/?] Add support for lowering variadic indirect calls.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:182
+    // now we don't add this part.
+    Constant *AddC = ConstantInt::get(I64Ty, RetStack ? 31 : 15);
+    Constant *NegC = ConstantInt::get(I64Ty, -16ll);
----------------
Maybe make the contribution from RetStack a bit more clear.


================
Comment at: llvm/test/CodeGen/AArch64/arm64ec-cfg.ll:491-492
+; CHECK-NEXT:    sub sp, sp, #40
+; CHECK-NEXT:    sub x10, x29, #48
+; CHECK-NEXT:    sub x0, x29, #48
+; CHECK-NEXT:    mov x9, x25
----------------
bcl5980 wrote:
> Can be TODO also.
> `sub x0, x29, #48` rematerialize from copy x10
> and `fmov d0, x10` can't rematerialize so `sub x10, x29, #48` remain.
> How could we improve the reMaterializeTrivialDef to improve the code?
Not sure how we're ending up with two separate operations in the first place; I'd normally expect SelectionDAG CSE to kick in.


================
Comment at: llvm/test/CodeGen/AArch64/arm64ec-cfg.ll:363-365
+; CHECK-NEXT:    mov x8, sp
+; CHECK-NEXT:    sub x0, x8, x15, lsl #4
+; CHECK-NEXT:    mov sp, x0
----------------
bcl5980 wrote:
> efriedma wrote:
> > bcl5980 wrote:
> > > It looks Microsoft generate code：
> > > 
> > > ```
> > > sub         sp,sp,x15,lsl #4
> > > ```
> > > Does anyone know why we can't accept SP as input/ouput for the instruction?
> > > 
> > > ```
> > > def : Pat<(sub GPR64:$Rn, arith_shifted_reg64:$Rm),
> > >           (SUBSXrs GPR64:$Rn, arith_shifted_reg64:$Rm)>;
> > > ```
> > > 
> > > 
> > > 
> > The instruction used by the Microsoft compiler is SUBXrx64.  SUBSXrs can't refer to sp that way; the "lsl" is actually an alternate spelling of "uxtx".
> > 
> > We could add a specialized pattern specifically for subtraction operations where the first operand of the subtraction is a copy from sp.
> //We could add a specialized pattern specifically for subtraction operations where the first operand of the subtraction is a copy from sp.//
> Do you mean add pattern in td file?
> 
> ```
> def : Pat<(sub GPR64sp:$SP, arith_extended_reg32to64_i64:$Rm),
>           (SUBSXrx GPR64sp:$SP, arith_extended_reg32to64_i64:$Rm)>;
> ```
> If yes, we may need to add a new select function similar to arith_extended_reg32to64_i64 but remove check extend. Or can we do this on DAGCombine?
> 
> And I think we can do this on another patch.
I think you'd want a PatLeaf to specifically look for a CopyFromReg from sp, as opposed to changing the way we lower all subtraction operations, but yes, that's the idea.

(You could, alternatively, introduce a target-specific node in AArch64ISelLowering.h specifically to represent "sub sp, sp, xN, lsl #4", use it from LowerDYNAMIC_STACKALLOC, and write a pattern to lower it to SUBXrx64.)

And yes, please leave it for another patch.


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

https://reviews.llvm.org/D129727