User Details
- User Since
- May 22 2014, 1:24 PM (498 w, 3 d)
Mar 11 2023
Mar 6 2023
LGTM
LGTM
Mar 5 2023
LGTM
As noted in previous comments, it's hard to reliably pattern-match a sequence that is this long. We are missing several potential canonicalizations in IR, and I see at least one possible variant that is shorter in IR:
https://alive2.llvm.org/ce/z/CkQ433
Mar 3 2023
Patch updated:
Add a RUN line to the instcombine test to show with and without loop info test diff.
This looks close to me, but see inline comments on the tests, and please pre-commit those with the baseline CHECK lines, so we'll just show test diffs in this patch.
Ping.
Mar 2 2023
Patch updated:
- Match any pair of no-wrap subtracts.
- But only propagate "nsw" with restrictions.
Mar 1 2023
LGTM
Feb 28 2023
Patch updated:
Add restriction to freeze creation for unsigned ops because they are easier to determine as safe.
This is adding a lot of code to a function that's already too big. A helper function will make it easier to read.
Feb 27 2023
Patch updated:
Added TODO comment to allow this for any binop (currently bails out on div/rem/shift).
LGTM - thanks!
See inline comments for a small adjustment to the tests.
Patch updated:
When necessary, freeze the select condition value to preserve the optimization.
We should have 2 more tests: (1) extra use of the sub and (2) extra use of the and.
As noted earlier, this patch should not require a "m_OneUse" limitation. I realize that it looks like a regression for the existing test, but that should be ok when viewed globally: we are reducing to an equality compare, and GVN, CVP, or some other pass will reduce that to the optimal form. It's just lucky (or unlucky) that the min/max folds added with D144606 are able to reduce it all within InstCombine.
Feb 26 2023
Patch updated:
- Use lambda and m_c_Binop to reduce code duplication (I tried a few variations of juggling the conditions - this was the best I could find).
- Added a pair of negative tests with mismatched demand/shuffle mask.
Feb 24 2023
Feb 23 2023
LGTM
LGTM
Patch updated:
Changed instcombine pipeline invocations to not use loop info unless it is already available, so this is effectively NFC.
I need LeafValues to access value by index. It looks SmallSetVector can't do that.
Feb 22 2023
Please rename the test file as a preliminary step, so we will again show diffs in this patch. We also need to add negative tests to show current limitations and also that the combining is not making wrong logic reductions.
Feb 21 2023
This seems close to ready to me. It would be great if other reviewers have a look too. :)
This doesn't handle i8 and i16 the same way because truncates get in the way, right? Would it be better to match this as "umax(x, 1)" (before it becomes x86-specific instructions)? Or could we peek through a truncate and still generate the adc for i8/i16?
Patch updated:
Reduced the impliesPoison() logic as suggested.
I don't know anything about ExecutionEngine - adding potential reviewers based on the most recent git log entries for this directory.
Feb 20 2023
We should probably view this as a missed canonicalization in IR first:
https://alive2.llvm.org/ce/z/D2Aph4 (that should also work with eq predicates)