Adds support for codegen of masked loads, with non-extending,
zero-extending and sign-extending variants.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Sam has been looking at extending masked loads and stores in D68337 and related patches. There looks like there would be some overlap with this, especially in the target independent parts. Make sure you co-ordinate with him.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
10465 | I'm not convinced that just because a sext load is legal and a masked load is legal, that a sext masked load is always legal. | |
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td | ||
1077 | What if the passthru isn't undef? | |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h | ||
151 | This can handle all masked loads? Of any type, extended into any other type, with any alignment? | |
153 | This patch doesn't handle stores yet. | |
llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
296 | Can this just use "undef"? |
- Rebased patch, removed extra sext & zext combine from DAGCombine which are no longer necessary
- Added isVectorLoadExtDesirable to AArch64ISelLowering
- Added more checks to isLegalMaskedLoad
- Changed SVEUndef to SVEDup0Undef, handling undef or all zeros
- Changed SelectionDAG::getConstant to return SPLAT_VECTOR instead of BUILD_VECTOR for scalable types
Thanks!
It looks like the only supported parameter of the PassThru here is a splat of 0 or undef. This might get in the way of IR level optimisation that could try to producing a masked load with different passthru, which would then fail to select. The ARM backed added a custom lowering to turn this case into a select of the loaded value and the passthru. Like this, which was further modified in later commits:
https://github.com/llvm/llvm-project/commit/b325c057322ce14b5c561d8ac49508adab7649e5#diff-b853ecd5d5e0f73c2c093ffb5b4f2fbbR8855
That way you only have to check on the zero, not undef. I'm not sure if there is support yet for vector selects in the SVE codegen?
I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?
llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
---|---|---|
4753 | Can you explain why is this pseudo is needed, exactly? I feel that using pseudos is often the wrong solution to a problem (it may be required here, I'm just sure why exactly). We currently seem to generate ld1b (for example), over ldnf1b. Is there ever a time that we expect to generate a nf load? |
There is not yet support for vector selects, so for this patch the intention was that any passthru which is not all zero or undef would result in a selection failure.
Do you think it would acceptable to handle different passthrus in a future patch which also implements vector selects for SVE?
llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
---|---|---|
4753 | The pseudo was a workaround that was added downstream for non-faulting loads, but it is not needed here. |
I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?
It is; we will need to set the operation action for supported VTs to 'Custom' once we have codegen for vselects in order to handle any passthru values which aren't undef or zero (which you've already noticed as missing in the review), but the operation itself is legal once it gets through type legalization.
There's a separate mechanism to handle illegal masked loads/stores in the ScalarizeMaskedMemIntrin pass before lowering to a SelectionDAG. Scalarization needs to create new basic blocks for a loop over the vector elements, which isn't easy after lowering. The TargetTransformInfo::isLegalMaskedLoad (or Store) functions are what determine whether scalarization takes place or not for a given target.
Yeah OK. That makes sense. I remember the same chicken and egg from MVE. We ended up doing them the other way around there. Can you try and add a FIXME comment somewhere?
Otherwise LGTM.
llvm/lib/Target/AArch64/SVEInstrFormats.td | ||
---|---|---|
4753 | Righteo. We know where to find it if it turns out we do need them. |
I'm not convinced that just because a sext load is legal and a masked load is legal, that a sext masked load is always legal.