[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/LegalizeIntegerTypes.cpp:4794-4795
   APInt StepVal = cast<ConstantSDNode>(N->getOperand(0))->getAPIntValue();
-  SDValue Step = DAG.getConstant(StepVal.getZExtValue(), dl, NOutElemVT);
+  SDValue Step = DAG.getConstant(
+      StepVal.sextOrTrunc(NOutElemVT.getSizeInBits()), dl, NOutElemVT);
   return DAG.getStepVector(dl, NOutVT, Step);
----------------
frasercrmck wrote:
> paulwalker-arm wrote:
> > Is calling sextOrTrunc necessary? I would have thought you could just replace `getZExtValue()` with `getSExtValue()`.
> > 
> > That said with @frasercrmck previous patch to remove the size restriction of `STEP_VECTOR`'s operands I don't believe there's need for this function to extend the operand at all and so `N->getOperand(0)` can be passed directly to `getStepVector`.
> Yeah I think if the incoming step is legal (which it possibly isn't in theory -- in practice we always create a step with TypeToTransformTo?) then it should be possible to pass it straight through.
ok，let's use it directly and add assertion here.


================
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());
----------------
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?
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100812