[PATCH] D120953: [AArch64][SelectionDAG] Supports unpklo/hi instructions to reduce the number of loads

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11497
       !ISD::isUNINDEXEDLoad(N0.getNode()) ||
-      ((LegalOperations || VT.isVector() ||
+      ((LegalOperations || VT.isFixedLengthVector() ||
         !cast<LoadSDNode>(N0)->isSimple()) &&
----------------
Allen wrote:
> paulwalker-arm wrote:
> > Allen wrote:
> > > @paulwalker-arm  do you think it is ok to fix the crash of normal loads ? 
> > Sorry for the delay but I've been ponding this on and off.  I cannot shake the feeling that any "is vector" check here is artificial and so limiting it to only fixed length vectors also seems artificial.
> > 
> > Personally I think `isVector()` should be removed and any negative effects on code generation being the result of that target's implementation of `isVectorLoadExtDesirable()` likely being at fault.
> > 
> > That said, I understand this might be above and beyond the work you want to carry out so I guess you making the current restriction slightly less artificial is a nice step in the right direction.
> Thanks very much.
> If you agree, I can add a comment "TODO: isFixedLengthVector() should be removed and any negative effects on code generation being the result of that target's implementation of isVectorLoadExtDesirable()", and try to refactor that with a separate commit.
@Allen I'm happy with this change.  It doesn't actually fix the bug that caused the crash but does maintain existing behaviour so that buggy code path is not hit (at least with the current set of tests).

The bug itself is within `DAGCombiner::CombineExtLoad` whereby is has a `!DstVT.isScalableVector()` assert that appears after a call to `getVectorNumElements()` so the code will fail before ever hitting that assert.  The real fix is to remove the assert and just add
```
if (DstVT.isScalableVector())
  return SDValue();
```
to the top of `DAGCombiner::CombineExtLoad`. As you've worked round the issue it's not strictly necessary but if you could add that fix to this patch then it'll stop anybody else from hitting it.


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

https://reviews.llvm.org/D120953