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

[Allen zhong via Phabricator via llvm-commits]

Allen marked 2 inline comments as done.
Allen added a comment.

In D120953#3396109 <https://reviews.llvm.org/D120953#3396109>, @paulwalker-arm wrote:

> Thanks @Allen this works for me.  Before accepting I just wanted to double check your previous comment:
>
> In D120953#3377136 <https://reviews.llvm.org/D120953#3377136>, @Allen wrote:
>
>> Thanks very much, this version I firstly fix the crash as you mention, and this version only play as the beginning of such optimisation.
>>
>> If you agree, then I'll refact with your advice on the next version (**it is safest to exclude all scalable vector extending loads/truncating stores and then selectively enable those which we directly support.** ), and add more cases to guard them.
>
> Is this something you can do for this patch? Or are you wanting to save that work for a follow on patch?

Yes, I hope this patch can be accepted firstly. Then I'll start another patch to finish the refactor.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11497
       !ISD::isUNINDEXEDLoad(N0.getNode()) ||
-      ((LegalOperations || VT.isVector() ||
+      ((LegalOperations || VT.isFixedLengthVector() ||
         !cast<LoadSDNode>(N0)->isSimple()) &&
----------------
@paulwalker-arm  do you think it is ok to fix the crash of normal loads ? 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11497
       !ISD::isUNINDEXEDLoad(N0.getNode()) ||
-      ((LegalOperations || VT.isVector() ||
+      ((LegalOperations || VT.isFixedLengthVector() ||
         !cast<LoadSDNode>(N0)->isSimple()) &&
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11506
     DoXform = ExtendUsesToFormExtLoad(VT, N, N0, ExtOpc, SetCCs, TLI);
-  if (VT.isVector())
+  if (VT.isFixedLengthVector())
     DoXform &= TLI.isVectorLoadExtDesirable(SDValue(N, 0));
----------------
paulwalker-arm wrote:
> However, the `isVectorLoadExtDesirable()` question will always be pertinent regardless of vector type and so you shouldn't need this change.
Done, thanks


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

https://reviews.llvm.org/D120953