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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

In D122703#3418244 <https://reviews.llvm.org/D122703#3418244>, @Allen wrote:

> hi @paulwalker-arm
>
>   With some debug with function DAGCombiner::visitAND, I find there is another issue need to check:
>
> 't s
> 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.

Not sure I understand what you mean here. `ExtVT` will be the legal type, most likely `MVT::nxv2i64`, whereas `MVT::nxv2i32` will be the MemVT that doesn't need to be legal, so I don't see an issue?  Looking at  `DAGCombiner::visitAND` it looks like it already does a related transformation but because it relies on `BuildVectorSDNode` is fails for scalable vectors.  To me it looks like we can make this common code more portable and support all vector types.



================
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();
----------------
Allen wrote:
> 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 ?
The need to check the extension type depends on which of the options you go with. If you only implement (1) then you need to check the extension type is explicitly zero-extending. Options (1) & (2) require you to skip sign-extending cases and for (3) you'll still need a use count check for the sign-extending case.


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

https://reviews.llvm.org/D122703