Skip to content

Document GlobalISel's combiner guidelines #92309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aemerson opened this issue May 15, 2024 · 5 comments
Open

Document GlobalISel's combiner guidelines #92309

aemerson opened this issue May 15, 2024 · 5 comments

Comments

@aemerson
Copy link
Contributor

@tschuett made a good point that our approach to writing combiners in GlobalISel needs some documentation here: #91922

This issue is to track that initial work on elaborating things like:

  • Combiner best practices
  • Rationales for what we do
  • Artifact vs regular combiner
  • Notable exceptions to the above and why they're there
@tschuett
Copy link

I vote for a section about the combiner design. We compile the Combine.td into a matchable. 10 more lines.

Complex logic is delegated to CombinerHelper Helper.

From this design, we can conclude:

  • Keep as much logic as possible in Combine.td and less logic in the Helper => lower compile time.
  • visit functions are an anti pattern. They delegate too much logic into the helper => higher compile time.
  • Many tight, precise, and small combines for lower compile time and targets can pick the choices.

tschuett added a commit to tschuett/llvm-project that referenced this issue May 16, 2024
@tschuett
Copy link

We are not rebuilding InstCombine. We take inspirations from InstCombine. If there are semantic uncertainties, we take InstCombine as the reference.

@tschuett
Copy link

tschuett commented Aug 2, 2024

Assuming the combiner is at least an artefact combiner, the AArch64 post legalization combiner is too weak, see
#101327 (comment)

@tschuett
Copy link

tschuett commented Aug 2, 2024

We should/must combine ADDEs post legalization, but we don't.

@tschuett
Copy link

To make a secret public, I have some interest in https://reviews.llvm.org/D96031. If this is a legacy way of doing combines and the rules changed, we should document that and make the rules public.

#90618 is plain port from InstCombine, which I believe was a great idea and big win for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants