This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Select unmasked RVV pseudos in a DAG post-process
ClosedPublic

Authored by frasercrmck on Feb 2 2022, 9:29 AM.

Details

Summary

This patch drops TableGen patterns matching all-ones masked RVV pseudos
in the case where there are fallback patterns matching the generic
masked forms to "_MASK" pseudos. This optimization is now performed with
a SelectionDAG post-processing step which peephole-optimizes these same
pseudos with all-ones masks and swaps them out to their unmasked
pseudos.

This cuts our generated ISel table down by around ~5% (~110kB) in lieu
of a far smaller auto-generated table to help with the peephole.

This only targets our custom RISCVISD::*_VL binary operator nodes, which
use the one form for both masked and unmasked variants. A similar
approach could be used for our intrinsics but we'd need to do some work,
e.g., to represent unmasked intrinsics as true-masked intrinsics at the
IR or ISel level. At a rough estimate, this could save us a further 9%
on the size of our ISel table for the binary intrinsic patterns alone.

There is no observable impact on our tests.

Diff Detail

Event Timeline

frasercrmck created this revision.Feb 2 2022, 9:29 AM
frasercrmck requested review of this revision.Feb 2 2022, 9:29 AM

remove debug assertion

craig.topper added inline comments.Feb 2 2022, 10:46 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2190

If the policy Op exists and isn't TAIL_AGNOSTIC, this transform isn't valid is it?

Using an all ones mask with TAIL_UNDISTURBED is a valid way to get tail undisturbed behavior until we have tail undisturbed unmasked instructions.

only optimize TAIL_AGNOSTIC, and make clang-format happy

frasercrmck marked an inline comment as done.Feb 3 2022, 7:48 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2190

Yes you're right, thanks, I wasn't careful enough about that. Though I suppose TAIL_UNDISTURBED with an IMPLICIT_DEF merge operand would still be valid to transform? Regardless, we don't have test coverage for that so I've not added logic to deal with that.

craig.topper added inline comments.Feb 3 2022, 11:45 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2190

I think the IMPLICIT_DEF case would be ok. Though maybe DAG combines to simplify such cases might be better.

frasercrmck marked an inline comment as done.Feb 4 2022, 4:59 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2190

Were you thinking of something that would DAG combine TU+IMPLICIT_DEF -> TA?

LGTM

Was this meant to be formal acceptance, sorry?

  • rebase and fix conflicts with widening patterns
This revision is now accepted and ready to land.Feb 8 2022, 10:19 AM
This revision was landed with ongoing or failed builds.Feb 9 2022, 12:00 AM
This revision was automatically updated to reflect the committed changes.