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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck 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);
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100812