[PATCH] D111633: [SelectionDAG] Fix getVectorSubVecPointer for scalable subvectors.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7822-7824
+      assert(IdxC->getZExtValue() + NumSubElts <= NElts &&
+             "Expected the trivial case to have been handled");
+      return Idx;
----------------
paulwalker-arm wrote:
> Given you're not making assumptions as to where `Idx` is coming from I don't think an assert is safe enough.  Sure an asserts build will exit here but a release build could leak/corrupt data, which is a problem the user is trying to prevent, hence calling this function.
> 
> Instead I think the assert should be replaced by always clamping `Idx` (see the `MaxIndex` calculation below).  If the source of `Idx` is indeed an `EXTRACT_SUBVECTOR/INSERT_SUBVECTOR` then the invalid index should be asserting as part of `getNode` rather than getting this far.
Good point about the generated code possibly doing the wrong thing for non-assert builds. I've changed it to always clamp, and removed the assert. From looking at the uses of `getVectorSubVecPointer`, it's not really worth passing in some bool to tell whether the value is coming from an insert/extract subvector in order to assert. The nodes where this function is called for insert/extract subvectors, have nodes that must already have been checked for correctness in other places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111633