[PATCH] D100812: [DAGCombiner] Allow operand of step_vector to be negative.

[JunMa via Phabricator via llvm-commits]

junparser added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3548
+  // canonicalize (sub X, step_vector(C)) to (add X,  step_vector(-C))
+  if (N1.getOpcode() == ISD::STEP_VECTOR) {
+    SDValue NewStep = DAG.getConstant(-N1.getConstantOperandAPInt(0), DL,
----------------
sdesmalen wrote:
> If there are multiple uses of step_vector(C), then it may be more beneficial to have a single step_vector(C) and use separate add/sub. Can you add a check that N1 has only a single use?
will update later.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1658-1665
+  APInt StepVal = cast<ConstantSDNode>(Step)->getAPIntValue();
   SDValue StartOfHi =
-      DAG.getVScale(dl, EltVT,
-                    cast<ConstantSDNode>(Step)->getAPIntValue() *
-                        LoVT.getVectorMinNumElements());
-  StartOfHi = DAG.getZExtOrTrunc(StartOfHi, dl, HiVT.getVectorElementType());
+      DAG.getVScale(dl, EltVT, StepVal * LoVT.getVectorMinNumElements());
+  StartOfHi =
+      StepVal.isNonNegative()
+          ? DAG.getZExtOrTrunc(StartOfHi, dl, HiVT.getVectorElementType())
+          : DAG.getSExtOrTrunc(StartOfHi, dl, HiVT.getVectorElementType());
----------------
junparser wrote:
> frasercrmck wrote:
> > paulwalker-arm wrote:
> > > This doesn't look correct to me.  The operand is either signed or unsigned and should be treated accordingly.  Given this patch wants to allow negative value then we're essentially converting the STEP_VECTOR to take a signed operand and should always use SExt.
> > > 
> > > That said I'm wondering if the issue here is using DAG to do the extension even though the operand is defined to be a constant.  Perhaps the following works for you?
> > > ```
> > > SDValue StartOfHi =
> > >       DAG.getVScale(HiVT.getVectorElementType(), StepVal * LoVT.getVectorMinNumElements());
> > > ```
> > I agree, I think it should be consistently SExt. I think @junparser tried to mitigate a RISC-V regression but in line with what you're saying, I bet some of the poor RISC-V code comes from the DAG extending VSCALE and not being able to optimize it due to not being able to infer the known bits.
> @paulwalker-arm,  The tradeoff here does try to mitigate regression in riscv-32 with element type like i64.  Meanwhile, extension is necessary for vscale(i64) under  riscv-32.  I checked PromoteIntRes_VSCALE which does not do truncation.  @frasercrmck, would it be ok to relax vscale as same as step_vector?
>  
@paulwalker-arm, The tradeoff here does try to mitigate regression in riscv-32 with element type like i64. Meanwhile, extension is necessary for vscale(i64) under riscv-32. Since there is no ExpandIntegerResult with vscale. @frasercrmck, would it be ok to relax vscale as same as step_vector?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100812