[PATCH] D92760: [SelectionDAG] Implement SplitVecOp_INSERT_SUBVECTOR

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2287-2291
+  if (ResVT.isFixedLengthVector() &&
+      N->getOperand(0).getValueType().isFixedLengthVector() &&
+      N->getOperand(1).getValueType().isScalableVector())
+    report_fatal_error("Inserting a scalable vector into a fixed-length vector "
+                       "is not yet supported. ");
----------------
joechrisellis wrote:
> paulwalker-arm wrote:
> > Is this protection necessary?  From what I can see the code below should work for all valid forms of INSERT_SUBVECTOR. That's to say you don't need to worry about the case of inserting a scalable vector into a fixed length vector because that is not a valid use of INSERT_SUBVECTOR and thus should be caught before getting here.
> It might not be necessary, but since `SplitVecOp_EXTRACT_SUBVECTOR` has a similar check I would like to keep this for the time being!
I believe in `SplitVecOp_EXTRACT_SUBVECTOR`'s case there is a check for a legitimate scenario that if hit may require an update to the function.  However, in your instance the check is simply validating the DAG, which is not necessary because that's the job of `SelectionDAG::getNode`, of which I can see:
```
assert((VT.isScalableVector() || N2VT.isFixedLengthVector()) &&                 
           "Cannot insert a scalable vector into a fixed length vector!");  
```
So unless the check serves a different purpose the code looks redundant and redundant code should be removed.  I guess if you are super paranoid you could replicate `SelectionDAG::getNode`'s assert but I really don't see the need.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92760