Skip to content
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

[InstCombine] Add contributor guide #79007

Merged
merged 8 commits into from
Mar 21, 2024
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jan 22, 2024

Document expectations for contributions to InstCombine, especially regarding test coverage and alive2 proofs.

@nikic nikic force-pushed the instcombine-contibutor-guide branch 2 times, most recently from 706f745 to fdfede6 Compare March 14, 2024 16:42
@nikic nikic force-pushed the instcombine-contibutor-guide branch from fdfede6 to a627fac Compare March 15, 2024 14:04
@nikic nikic marked this pull request as ready for review March 15, 2024 14:04
@dtcxzyw dtcxzyw requested a review from arsenm March 15, 2024 14:38
* `m_OneUse(M)`: Check that the value only has one use, and also matches M.
For example `m_OneUse(m_Add(...))`. See the next section for more
information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: m_ElementwiseBitCast. It may confuse contributors when folding bitcast-related patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems pretty exotic to me, compared to the other ones.

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 18, 2024

@AtariDreams @elhewaty @ParkHanbum @SahilPatidar
Any thoughts?

@elhewaty
Copy link
Contributor

we added this:

If the simplification creates new instructions, it falls into the InstCombine part. You should find the root of the pattern and look through InstCombinerImpl::visitXXXInst.

but I sill find the look through InstCombinerImpl::visitXXXInst part for the new contributors.

@elhewaty
Copy link
Contributor

what about adding when to use return replaceInstUsesWith, return Builder.CreateXX, and return new SomeInstruction?

@goldsteinn
Copy link
Contributor

One more thing that might be nice is an example or so of successful PRs new contributors can look at to get a sense of how all these rules play out.
Think: #82929 and/or #78395 are pretty easy to follow and checks all the boxes.

Either way though, I think at this point the document is clearly useful and would be happy with it going up in its current form. We can always add more later.

@nikic
Copy link
Contributor Author

nikic commented Mar 19, 2024

what about adding when to use return replaceInstUsesWith, return Builder.CreateXX, and return new SomeInstruction?

Done.

@nikic
Copy link
Contributor Author

nikic commented Mar 21, 2024

Any more feedback on this?

Make tests minimal. Only test exactly the pattern being transformed. If your
original motivating case is a larger pattern that your fold enables to
optimize in some non-trivial way, you may add it as well -- however, the bulk
of the test coverage should be minimal.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Drop unused poison-generating flags/UB-implying attributes.

One-use checks can be performed using the `m_OneUse()` matcher, or the
`V->hasOneUse()` method.

### Generalization
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Be aware of poison-generating flags/FMF propagation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example: #72127

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic
Copy link
Contributor Author

nikic commented Mar 21, 2024

I'll go ahead and merge this so we can iterate further in-tree.

@nikic nikic merged commit 6898147 into llvm:main Mar 21, 2024
5 checks passed
@nikic nikic deleted the instcombine-contibutor-guide branch March 21, 2024 15:00
@ParkHanbum
Copy link
Contributor

ParkHanbum commented Mar 22, 2024

thanks for work to this!! I was very lost when I first contributed to LLVM and this will help me a lot.

Q. when I can see this document from web site llvm.org?

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Document expectations for contributions to InstCombine, especially
regarding test coverage and alive2 proofs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants