This is an archive of the discontinued LLVM Phabricator instance.

Tests + Improve cases for optimizing out some icmp(binop) patterns (mostly mul)
AbandonedPublic

Authored by goldstein.w.n on Jan 2 2023, 2:09 AM.

Details

Reviewers
majnemer
nikic
Summary
Commit 1 (Tests):

Add tests for binops with conditions/assume constraints

Commit 2 (Code Changes):

Improve cases for optimizing out some icmp(binop) patterns (mostly mul)

  1. Add some explicit cases for mul that were missing.
  2. Improve ValueTracking analysis to better track the ICMP_NE and ICMP_ULT patterns.
    • Both for tracking on edges of BBs and for conditions that are used by llvm.assume

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 2 2023, 2:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2023, 2:09 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Jan 2 2023, 2:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2023, 2:09 AM

ran alive2 on all transform.

Transforms/InstCombine/pr38677.ll: https://alive2.llvm.org/ce/z/atn3gZ

$> alive-tv --smt-to=2500000 BEFORE-icmp-binop.ll AFTER-icmp-binop.ll
...
Summary:

146 correct transformations
0 incorrect transformations
0 failed-to-prove transformations
0 Alive2 errors

The revision has two commits, I created the revision with:

$> arc diff --create HEAD^^

Although it seems to have chosen the commit header for the 1st commit (tests before the change)
and the entire second commit message for the summary.

Sorry about that. I think I need to modify the message in arc diff when doing multi-commit
patches in the future.

goldstein.w.n retitled this revision from Add tests for binops with conditions/assume constraints to Tests + Improve cases for optimizing out some icmp(binop) patterns (mostly mul).Jan 2 2023, 2:14 AM
goldstein.w.n edited the summary of this revision. (Show Details)
nikic requested changes to this revision.Jan 2 2023, 2:30 AM
nikic added a subscriber: nikic.

This patch is mixing up a lot of different changes. Please split it up into self-contained changes to the degree that this is possible. I would recommend starting with just your changes in InstCombineCompares, as these look like they should be able to stand on their own (without changes to AssumptionCache machinery).

This revision now requires changes to proceed.Jan 2 2023, 2:30 AM

This patch is mixing up a lot of different changes. Please split it up into self-contained changes to the degree that this is possible. I would recommend starting with just your changes in InstCombineCompares, as these look like they should be able to stand on their own (without changes to AssumptionCache machinery).

Sure, should I just post back to this one with the changes split into more commits or create a new review starting with
the InstCombineCompares change?

nikic added a comment.Jan 2 2023, 5:26 AM

This patch is mixing up a lot of different changes. Please split it up into self-contained changes to the degree that this is possible. I would recommend starting with just your changes in InstCombineCompares, as these look like they should be able to stand on their own (without changes to AssumptionCache machinery).

Sure, should I just post back to this one with the changes split into more commits or create a new review starting with
the InstCombineCompares change?

Locally you'd probably want to split into commits, but as far as Phabricator is concerned they should be separate reviews -- Phabricator does not allow reviewing individual commits in a review. I don't know how one does that through arc, I always upload patches via the web interface.

This patch is mixing up a lot of different changes. Please split it up into self-contained changes to the degree that this is possible. I would recommend starting with just your changes in InstCombineCompares, as these look like they should be able to stand on their own (without changes to AssumptionCache machinery).

Sure, should I just post back to this one with the changes split into more commits or create a new review starting with
the InstCombineCompares change?

Locally you'd probably want to split into commits, but as far as Phabricator is concerned they should be separate reviews -- Phabricator does not allow reviewing individual commits in a review. I don't know how one does that through arc, I always upload patches via the web interface.

Okay, I split it into 4 commits and 4 separate reviews:

Tests

https://reviews.llvm.org/D140849

InstCombineCompares

https://reviews.llvm.org/D140850

AssumptionCache

https://reviews.llvm.org/D140851

isKnownNonZero

https://reviews.llvm.org/D140852

Is there a way to close this review?

@nikic This is somewhat unrelated so if this is not the place to ask, let me know.

I see your commit:

[InstCombine] Make TTI comment more forceful (NFC)

To save people not familiar with InstCombine from creating patches
that are immediately rejected by policy.

I was hoping to make a patch to avoid the de-optimization
that occurs in ShrinkDemandedConstant when the reduced
bits does something like break imm8 sign extension and forces
an imm64 (causes movabs for X86, ot movk + lsl/ldq on
aarch64).

I figured the best way to do this was with getIntImmCostInst
but your comment is pretty forceful.

Is this an acceptable use case that could warrant an exception?
If not, do you have an advise for how to avoid this de-optimization?

goldstein.w.n abandoned this revision.Jan 12 2023, 11:36 AM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp