This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Add two optimizations for mul-and-icmp patterns
AbandonedPublic

Authored by fwolff on Nov 19 2021, 12:40 PM.

Details

Summary

Fixes PR#40493, and in principle also PR#51551, but the known bits analysis isn't quite smart enough to handle the assume in the example there.

The entire logic for this patch has been discussed in PR#40493, it just seems that nobody has gotten around to implementing it yet.

https://alive2.llvm.org/ce/z/Bp6ZoV
https://alive2.llvm.org/ce/z/htSh_Q

Diff Detail

Event Timeline

fwolff created this revision.Nov 19 2021, 12:40 PM
fwolff requested review of this revision.Nov 19 2021, 12:40 PM
lebedev.ri added a subscriber: lebedev.ri.

alive2 proofs?

llvm/test/Transforms/InstCombine/pr40493.ll
1 ↗(On Diff #388596)

Precommit tests please

alive2 proofs?

@test1: https://alive2.llvm.org/ce/z/BN74k2
@test2: https://alive2.llvm.org/ce/z/3PRnmg
@test2_2: https://alive2.llvm.org/ce/z/FmrCSL

The other tests don't do this transformation.

llvm/test/Transforms/InstCombine/pr40493.ll
1 ↗(On Diff #388596)

Do you mean that I should submit this file separately first in a different revision?

alive2 proofs?

@test1: https://alive2.llvm.org/ce/z/BN74k2
@test2: https://alive2.llvm.org/ce/z/3PRnmg
@test2_2: https://alive2.llvm.org/ce/z/FmrCSL

The other tests don't do this transformation.

This only shows that those tests aren't miscompiled, i'm interested in the generalized proof for the actual preconditions.

RKSimon added inline comments.Nov 19 2021, 2:23 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
1897

(style) Don't use auto - APInt should be enough

1911–1912

Don't use auto

This only shows that those tests aren't miscompiled

I know; I thought that's what you wanted because the transformations themselves have already been checked in PR#40493, although the links now appear to be defunct.

Anyway, here are the proofs:
https://alive2.llvm.org/ce/z/Bp6ZoV
https://alive2.llvm.org/ce/z/htSh_Q

fwolff updated this revision to Diff 388632.Nov 19 2021, 2:38 PM
fwolff marked 2 inline comments as done.

vector tests?

lebedev.ri edited the summary of this revision. (Show Details)Nov 26 2021, 2:38 AM

Thank you for looking into this.

vector tests?

Please add vector tests (with splat constants (all constant elements being the same),
splat with undef (there are 2 constants, so 3 variants - same undef lane in either of the constants, and in all of them)),
non-splat, commit these baseline tests into the llvm repo, and rebase this patch - the diff here should not show the tests as being added, it should show the check lines as being changed.

Note: the existing transform to trunc was removed with f55d1eb3746a , so this patch will need to be updated to account for that.

I suspect we need a "hasOneUse" check on the multiply on at least the first transform in this patch, but we should have tests for those patterns either way. It would be easier to review all the permutations if the tests are pre-committed, and this patch is split in 2 pieces (one for each transform).

spatel added inline comments.Nov 28 2021, 7:23 AM
llvm/test/Transforms/InstCombine/pr40493.ll
1 ↗(On Diff #388596)

This was answered in another comment, but to clarify:

Yes - this means (1) create the tests, (2) auto-generate the baseline (without this code change) CHECK lines, (3) commit that, and (4) rebase this patch so we just see the diffs from the baseline results in this review.

I prefer a pattern-based rather than PR-based file name (it may be possible to just add the tests to an existing file with tests for related transforms). It's also nicer to have descriptive function names rather than the anonymous @test....

Ideally, you can push the baseline tests to the main branch. Have you requested/received commit access yet?
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

@fwolff reverse ping?
This doesn't need any big fundamental changes, just a little rebasing.

fwolff updated this revision to Diff 392889.Dec 8 2021, 12:47 PM

@fwolff reverse ping?
This doesn't need any big fundamental changes, just a little rebasing.

Yes, sorry for the delay. I've committed the tests and updated the diff now.

fwolff updated this revision to Diff 400671.Jan 17 2022, 4:43 PM

Rebased again. Ping @lebedev.ri

The test coverage still seems lacking to me, and I'm not seeing why we need to create a new multiply. If we assume the right constraints, the first transform should be only a low-bit check?
https://alive2.llvm.org/ce/z/YB_4pn

I'll add some tests and post a draft that handles that part of the problem.

@fwolff Please can you rebase this patch?

fwolff updated this revision to Diff 406911.Feb 8 2022, 11:43 AM

@fwolff Please can you rebase this patch?

Done!

Of the remaining test diffs:
(1) mul_mask_pow2_ne0_use1 is a multi-use corner case that was added to confirm that we don't go overboard
(2) pr51551 and pr51551_2 are handled with a combination of -bdce and -instcombine: https://godbolt.org/z/xr1e96ceM

Do we still need this patch?

We now handle all of the motivating cases with demanded bits improvements ( 995d400f3a3c ), so this patch can be abandoned.

RKSimon resigned from this revision.Feb 13 2022, 3:03 AM

This patch should be unnecessary now

xbolva00 resigned from this revision.Mar 12 2022, 6:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2022, 6:05 AM
fwolff abandoned this revision.Mar 20 2022, 6:06 PM