This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Optimize new cases for lowerConvertToSVBool
ClosedPublic

Authored by alban.bridonneau on May 4 2022, 1:23 AM.

Details

Summary

Converts to SVBool are already considered as a nop, if they
are converting an operand from a ptrue or a cmp, because
they zero the extra predicate lanes by construction.

This patch adds 2 similar cases:

  • The wide cmp, which were not directly recognized by the test

for other forms of cmp

  • Splats of 1, which will be generated as ptrue, and as such

will also zero the extra predicate lines.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 1:23 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
alban.bridonneau requested review of this revision.May 4 2022, 1:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 1:23 AM
mgabka added a subscriber: mgabka.May 4 2022, 1:31 AM

Just adding [SVE] to the commit message

alban.bridonneau retitled this revision from Optimize new cases for lowerConvertToSVBool to [SVE] Optimize new cases for lowerConvertToSVBool.May 4 2022, 1:37 AM
paulwalker-arm added inline comments.May 4 2022, 3:31 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4193

In this context I don't believe isAllActivePredicate is safe to use. isAllActivePredicate will look through some AArch64ISD::REINTERPRET_CAST nodes because they promise not to use the "invisible" lanes they produce. For this code path you're asking the opposite question of "are the invisible lanes known to be zero". Using isAllActivePredicate in this way will allow the invalid transformation toSVBool(reinterp_nxv2(ptrue_s)) -> ptrue_s. I think you instead need to check InOp is specifically the "splat of 1" node (i.e. ISD::isConstantSplatVectorAllOnes(...)).

If you agree please add a negative test so that we can validate the current behaviour.

I've changed the code to only explicitely check for splats of 1, rather
than all forms of all active predicates.
I also added the requested negative unit test. Note that this doesn't
actually guard the code that we just changed, because the to.bool is
lowered before the from.bool, so the from.bool has not yet been lowered
to reinterpret cast at this point. I kept the unit test anyway,
because this is a valid case to be tested.

Matt added a subscriber: Matt.May 4 2022, 12:27 PM
peterwaller-arm accepted this revision.May 5 2022, 1:50 AM

LGTM with a naming nit, please leave others time to chime in before submitting as usual.

llvm/test/CodeGen/AArch64/sve-intrinsics-reinterpret.ll
105–106

If I understand, as you say this test is not exercising REINTERPRET-of-REINTERPRET at the ISD level, can it be reworded to avoid 'reinterpret' and potentially being misleading?

It seems good to me to avoid reference to internal details here (which can be misleading or might change).

This revision is now accepted and ready to land.May 5 2022, 1:50 AM

I've changed the name for the unit test

paulwalker-arm accepted this revision.May 6 2022, 7:21 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4177

Can you use InOp.getConstantOperandVal(0) directly here? because otherwise the next person to add an entry is going to get compiler warnings about the liveness of IntNo crossing case boundaries. You could fix this by adding extra {} but given this is the only use of IntNo there doesn't seem much value in keeping it.

I've refactored the switch statement as proposed.

The patch has been submitted. Somehow this revision wasn't closed automatically