Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
For tests, https://developers.redhat.com/articles/2022/12/20/how-contribute-llvm#writing_tests should give you a pretty good idea about our expectations for InstCombine tests. There will be quite a few "commuted" tests for this pattern (which your current implementation does not fully handle). You'll also want multi-use tests (which you need to guard against via one-use checks), tests with nuw/nsw flags (to test flag preservation) and negative tests (where the fold does not occur).
Please also attach alive2 proofs for the transform. I'd recommend splitting the FP part of this into a separate patch, as different people may need to look at that one.
alive2: https://alive2.llvm.org/ce/z/ri5iFw
I've left out the FP part for now, as you suggested - and am going to focus on testing now. That should hopefully reveal the cases that aren't covered yet. Thanks!
This includes only the positive tests, not completely sure which specific negative cases you had in mind.
I'm not entirely sure how to proceed with the single-use stuff and please let me know how things like those static functions, which are only used in one other function, are handled in LLVM/InstCombine. I've tried storing and reusing the BinaryOp_matches themselves, but that's apparently not possible. Would you prefer lambda functions or internal structs with static functions? Are you happy with relying on || short circuiting, or should that always be a separate if-else? Please let me know :)
Things like: Shit amount is not 1. Values that must be the same are not the same.
I'm not entirely sure how to proceed with the single-use stuff
From the testing side, you will want to pass one/both of the arguments to the final add to a call void @use(i32 %abc) to add an extra use. On the implementation side, you'll want to use m_OneUse().
and please let me know how things like those static functions, which are only used in one other function, are handled in LLVM/InstCombine. I've tried storing and reusing the BinaryOp_matches themselves, but that's apparently not possible. Would you prefer lambda functions or internal structs with static functions?
Static functions or lambdas are fine. I doubt they are needed here though, if you use commutative matchers.
Are you happy with relying on || short circuiting, or should that always be a separate if-else? Please let me know :)
Short-circuiting is fine.
| llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
|---|---|---|
| 1014–1017 | Similar in other places. And then drop the static function as unnecessary. | |
| llvm/test/Transforms/InstCombine/add.ll | ||
| 3099–3110 | Or similar. Avoid unnamed values in tests. | |
| 3887–3888 | Please separate the test additions into a separate patch, and then base this one on top, so only the test diff is visible. See https://llvm.org/docs/TestingGuide.html#precommit-workflow-for-tests for more information. This is particularly important to verify that you commutation tests work correctly. | |
| llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
|---|---|---|
| 1018 | Since these are all very function specific helpers, can you make them lambdas instead? | |
| llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
|---|---|---|
| 1009–1010 | is there a nice way to do this? maybe a m_AOrB / m_OneOf or something? because otherwise I don't see how this can be part of a commutative match(&I, m_c_Add(..., ...)). Or am I missing something? | |
| llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
|---|---|---|
| 1009–1010 | You can use m_CombineOr | |
Maybe you need to create two patch.
The first patch includes your unoptimized testcases without the fold. (Your first commit)
The second patch includes optimized testcases and the code for the fold. (Your second commit, which is based on your first commit)
And then you should let the first patch be the parent revision of the second patch through "Edit Related Revisions".
| llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp | ||
|---|---|---|
| 1007 | This should be m_c_Add. | |
my clang-format didn't seem to complain about anything, but let's see how this one goes on the build server.
running clang-format on just this file seems to change way more for me than just the touched lines and git clang-format or git diff -U0 --no-color --relative main | clang-format-diff.py -p1 -i doesn't seem to change any of the files, so I'm a bit puzzled. However, I've removed two unused variables.
up-to-date with main, git-clang-format seemed to dislike CRLF, so I'm using LF now, let's hope buildkite clang-format now matches with my local clang-format.
That said, I'm still getting this behaviour:
$ git-clang-format HEAD~1 The following files would be modified but have unstaged changes: M llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp Please commit, stage, or stash them first. $ git status -uno Refresh index: 100% (132404/132404), done. On branch cstiller/patched0 nothing to commit (use -u to show untracked files) $ git-clang-format HEAD~1 changed files: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp $ git status -uno Refresh index: 100% (132404/132404), done. On branch cstiller/patched0 nothing to commit (use -u to show untracked files)
clang-format appears to also be happy on buildkite now as well, sorry for the inconveniences. to me it looks like all failed tests are unrelated, so should be ready to be committed by someone with access. Thanks!
Are there any further changes required? Otherwise, please commit the patch for me; I don't have commit access atm.
This should be m_c_Add.