[PATCH] D109313: [SelectionDAG] PromoteIntRes_EXTRACT_SUBVECTOR for scalable vectors.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 3 inline comments as done.
sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4738
+      unsigned NElts = NInVT.getVectorMinNumElements();
+      uint64_t IdxVal = cast<ConstantSDNode>(BaseIdx)->getZExtValue();
+
----------------
CarolineConcatto wrote:
> It would not be best if NElts and IdxVal be the same type?
> Both should be unsigned or uint64_t.
> 
> In the same topic, why getZExtValue the index?
That shouldn't really matter. `IdxVal` must be `uint64_t` because of the value returned by `getZExtValue()`.
Any uses of `NElts` will be promoted to the widest type, which in case of `IdxVal` will be a 64-bit unsigned value type.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4741
+      SDValue Step1 = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, NInVT, InOp0,
+                                  DAG.getConstant((IdxVal / NElts) * NElts, dl,
+                                                  BaseIdx.getValueType()));
----------------
craig.topper wrote:
> CarolineConcatto wrote:
> > Should this be:
> >   DAG.getConstant((IdxVal, dl, ...
> > Because:
> > (IdxVal / NElts) * NElts =. IdxVal?
> Unless this trying to align IdxVal to a multiple of NElts <= IdxVal. In which case it might be clearer as alignDown(IdxVal, NElts)
Nice suggestion, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109313