[PATCH] D122703: [AArch64][InstCombine] Fold MLOAD and zero extensions into MLOAD

[Allen zhong via Phabricator via llvm-commits]

Allen added a comment.

hi @paulwalker-arm

  With some debug with function DAGCombiner::visitAND, I find there is another issue need to check:

it use **TLI.isLoadExtLegal(ISD::ZEXTLOAD, ExtVT, LoadVT)** to  check whether the following transform can be done, do we need similar check in the AArch64 backend? If yes, I think we must set MVT::nxv2i32 legal.
fold (and (masked_load) (build_vec (x, ...))) to zext_masked_load



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14422-14423
+
+    // If the mask is fully covered by the MLOAD, we don't need to push
+    // a new AND onto the operand
+    EVT EltTy = MemVT.getVectorElementType();
----------------
paulwalker-arm wrote:
> The masked load's MemVT is not sufficient as you must also consider the load's extension type.
> 
> 1. As it stand this transform is only safe for zero-extending masked loads...
> 2. for an any existing masked load the extension can be changed to zero-extending...
> 3. for a single use sign extending load the extension can be changed to zero-extending.
> 
> How far down this path you go depends on the exact use case you need.  If it's just (1) then I think this can be a common DAGCombine. For (2) and (3) it'll depend on whether there are sufficient TLI hooks to ensure you don't create a version a target cannot lower. That said, given you require the input to be an extending masked load perhaps that is not a problem.
> 
> One final issue is the handling of the masked load's pass through value.  With the `AND` in place this will be zero-extended but once removed that stops and thus that will need to be considered as well.  My guess is that for today's use case you want to restrict the combine to instances where the pass through value is either undef or zero.
Thanks @paulwalker-arm for detail comment. 
I has a question: Both 2) any extension and 3) sign extension can be changed to zero-extending, so why we need check the  load's extension type ?


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

https://reviews.llvm.org/D122703