This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement isMaskAndCmp0FoldingBeneficial hook
ClosedPublic

Authored by asb on Aug 9 2022, 6:27 AM.

Details

Summary

This hook is currently only used by CodeGenPrepare, which will sink an
and into a block that has an icmp 0 user of it.

This hook is less useful for RISC-V than for targets like AArch64 that
have a TBZ (test bit and branch if zero instruction), but may still be
profitable if Zbs is available and a BEXTI can be selected.

Conservatively, we return false even if Zbs is enabled for any masks
that fit in the ANDI immediate because it's possible the only use is a
branch on the result, and ANDI+BNEZ => BEXTI+BNEZ isn't a profitable
transformation.

Diff Detail

Event Timeline

asb created this revision.Aug 9 2022, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 6:27 AM
asb requested review of this revision.Aug 9 2022, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 6:27 AM
reames requested changes to this revision.Aug 9 2022, 8:12 AM
reames added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1172

This bit of logic feels like you're covering up something else. Moving around the and instruction in IR shouldn't directly cause the bexti to be used if the andi form is legal and profitable. Do we maybe have a missing pattern?

As a guess, maybe this is intersecting with D131482?

This revision now requires changes to proceed.Aug 9 2022, 8:12 AM
craig.topper added inline comments.Aug 9 2022, 9:28 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1172

This hook enables copying of the AND not just moving. If we're just going to emit an ANDI, it doesn't really make sense to enable copying it.

1172

'would bit' -> 'would fit'?

asb updated this revision to Diff 451388.Aug 10 2022, 1:50 AM
asb marked an inline comment as done.

Fix typo in comment and update it to better reflect the impact of the hook.

asb added inline comments.Aug 10 2022, 1:52 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1172

This hook is only used from CodeGenPrepare, to decide whether to sink an AND or not. I've updated the comment to hopefully better reflect the reasoning here.

Based on the only current in-tree use, a more descriptive name for this hook might be "shouldSinkAndToEnableMaskAndCmp0Folding".

reames accepted this revision.Aug 10 2022, 7:22 AM

LGTM

Please update the submission comment and code comment to emphasize the sink *and duplicate* part. Sinking to me is strictly code motion, not duplication. The restriction on andi only makes sense when you consider the duplication.

This revision is now accepted and ready to land.Aug 10 2022, 7:22 AM
This revision was landed with ongoing or failed builds.Sep 13 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.