This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] NFC: Move safe predicate casting to a separate function.
ClosedPublic

Authored by sdesmalen on Jun 30 2022, 9:04 AM.

Details

Summary

This patch puts the code to safely bitcast a predicate, and possibly zero
any undefined lanes when doing a widening cast, into one place and merges
the functionality with lowerConvertToSVBool.

This is some cleanup inspired by D128665.

Diff Detail

Event Timeline

sdesmalen created this revision.Jun 30 2022, 9:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 9:04 AM
sdesmalen requested review of this revision.Jun 30 2022, 9:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 9:04 AM
paulwalker-arm added inline comments.Jun 30 2022, 9:24 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4368–4369

InVT.bitsGT(VT)? assuming I've got my VTs in the correct order.

4571

Why is this change needed? This is the scenario when AArch64ISD::REINTERPRET_CAST is safe to use directly?

llvm/lib/Target/AArch64/AArch64ISelLowering.h
1156

Can this be just getSVEPredicateCast? because we'll never use this function in the context of a bitcast. Also in this context I think Safe can be assumed.

sdesmalen added inline comments.Jun 30 2022, 9:29 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4571

It's not strictly needed, but the function implements this behaviour so I figured we might just as well use it. I can also restrict getSVEPredicateBitCast to only "widening" casts.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4347–4348

This'll be because of my original rubbish code. Given this is a AArch64TargetLowering member function, you should be able to call isTypeLegal() directly.

4571

I'm not totally against its use here, I mean it does reduce the line wrapping :), I'm just a little conscious of compiler time creep.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
21493–21494

Please add InVT.getVectorElementType() != MVT::i1 && and remove the previous assert.

sdesmalen updated this revision to Diff 441647.Jul 1 2022, 2:28 AM
sdesmalen marked 6 inline comments as done.

Addressed review comments.

  • Renamed getSVESafePredicateCast -> getSVEPredicateCast
  • Use EVT::bitsGT instead of ElementCount::isKnownGT
  • Replaced TLI.isTypeLegal with isTypeLegal.
sdesmalen added inline comments.Jul 1 2022, 2:35 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4571

In that case, I prefer to keep it as-is.

paulwalker-arm accepted this revision.Jul 1 2022, 4:14 AM
This revision is now accepted and ready to land.Jul 1 2022, 4:14 AM
This revision was landed with ongoing or failed builds.Jul 4 2022, 3:58 AM
This revision was automatically updated to reflect the committed changes.