Page MenuHomePhabricator

[DAG] Add functionality for masked truncating store actions
AcceptedPublic

Authored by DavidTruby on Oct 26 2021, 7:11 AM.

Details

Summary

This adds functionality for setting and getting the relevant action
for masked truncating stores in the same way as for standard truncating
stores. There should be no change to code generation for any architecture
at this stage.

This support is needed to correctly implement masked truncating stores for
SVE on AArch64.

Diff Detail

Event Timeline

DavidTruby created this revision.Oct 26 2021, 7:11 AM
DavidTruby requested review of this revision.Oct 26 2021, 7:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2021, 7:11 AM

Is this needed for SVE support of masked truncating stores? My point in D108115 was that we could probably (until someone came along with an opposite position) treat masked truncating stores and truncating stores as symmetric. This will be true for MVE and SVE I think, and is true for X86 as far as I understand. (A second opinion on that would be good though). I imagine RiscV is the same with how their vector architecture works, not sure about hexagon. Sorry for not being very clear on that. Perhaps it's best to add anyway, but the equivalent code for zext/sext masked loads uses isLoadExtLegalOrCustom for the two and hasn't needed to split them into normal/masked variants: https://github.com/llvm/llvm-project/blob/e42f5d4b488e78ebf5b756e1e76422c7458ba81c/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L10917

My only comment on D108115 was a minor nitpick with formatting and a question about how the AArch64 code was working. RKSimon had a comment about the difficult to update tests too.

On a purely technical note, masked loads/stores only operate on vector types as far as I understand, and TruncMStoreActions doesn't appear to be initialized anywhere.

Add initialisation for TruncMStoreActions

Is this needed for SVE support of masked truncating stores? My point in D108115 was that we could probably (until someone came along with an opposite position) treat masked truncating stores and truncating stores as symmetric. This will be true for MVE and SVE I think, and is true for X86 as far as I understand. (A second opinion on that would be good though). I imagine RiscV is the same with how their vector architecture works, not sure about hexagon. Sorry for not being very clear on that. Perhaps it's best to add anyway, but the equivalent code for zext/sext masked loads uses isLoadExtLegalOrCustom for the two and hasn't needed to split them into normal/masked variants: https://github.com/llvm/llvm-project/blob/e42f5d4b488e78ebf5b756e1e76422c7458ba81c/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L10917

My only comment on D108115 was a minor nitpick with formatting and a question about how the AArch64 code was working. RKSimon had a comment about the difficult to update tests too.

On a purely technical note, masked loads/stores only operate on vector types as far as I understand, and TruncMStoreActions doesn't appear to be initialized anywhere.

Ah ok, well this still feels like the correct thing to do to me. With regards to the extends, I was also thinking of looking at that and splitting the extend logic in the same way in future as there's no logical reason that masked and non-masked versions of these should be symmetric either.

OK. We have got this far without needing a MLoad version, so SVE doesn't seem like it would necessitate adding an TruncMStore. Perhaps it is worth adding at any rate. It is conceptually cleaner, but does involve keeping extra state in TargetLowering. I will defer to the what others on review think.

peterwaller-arm accepted this revision.Oct 28 2021, 3:12 AM

LGTM, would it be a good idea to give this patch an [NFCI] tag before committing?

This revision is now accepted and ready to land.Oct 28 2021, 3:12 AM
paulwalker-arm added inline comments.Mon, Nov 1, 6:25 AM
llvm/include/llvm/CodeGen/TargetLowering.h
1302–1303

This comment does not make sense to me. What does "has solution" mean?

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9849–9855

Can you explain why these new DAG combines are required for what is supposed to be an NFC patch?

Taking another look I would expect this patch to also change LegalizeDAG to query TruncMStoreActions?

Matt added a subscriber: Matt.Mon, Nov 1, 2:31 PM