This is an archive of the discontinued LLVM Phabricator instance.

[KnownBits] Add support for X*X self-multiplication
ClosedPublic

Authored by RKSimon on Aug 31 2021, 6:13 AM.

Details

Summary

Add KnownBits handling and unit tests for X*X self-multiplication cases which guarantee that bit1 of their results will be zero - see PR48683.

https://alive2.llvm.org/ce/z/NN_eaR

The next step will be to add suitable test coverage so this can be enabled in ValueTracking/DAG/GlobalISel - currently only a single Analysis/ScalarEvolution test is affected.

Diff Detail

Event Timeline

RKSimon created this revision.Aug 31 2021, 6:13 AM
RKSimon requested review of this revision.Aug 31 2021, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 6:13 AM
nikic added a comment.Aug 31 2021, 6:23 AM

Isn't "self add" just a left shift by one? I don't think it makes sense to use computeForAddSub() for that.

nikic added a comment.Aug 31 2021, 6:26 AM

Though InstCombine already canonicalizes add x, x to shl x, 1, so it probably doesn't make sense to explicitly handle that case at all.

RKSimon updated this revision to Diff 369890.Sep 1 2021, 2:34 AM

Clean up the selfadd cases to just perform a shl-by-one.

InstCombine does canonicalize X+X to X<<1, but we have cases in DAG where this is reversed - such as the cases being addressed in PR50468/D108139 etc.

foad added a subscriber: foad.Sep 1 2021, 3:17 AM
foad added inline comments.
llvm/lib/Support/KnownBits.cpp
61

I'm not a huge fan of making this part of the existing computeForAddSub function, since it's a completely separate code path. How about a new computeForSelfAdd instead?

RKSimon added inline comments.Sep 1 2021, 8:24 AM
llvm/lib/Support/KnownBits.cpp
61

I'm going to drop the X+X case for now - and handle it as part of PR50468

RKSimon updated this revision to Diff 369941.Sep 1 2021, 8:38 AM
RKSimon retitled this revision from [KnownBits] Add support for X+X and X*X self-addition/multiplication to [KnownBits] Add support for X*X self-multiplication.
RKSimon edited the summary of this revision. (Show Details)

remove X+X handling - we'll address this later on

foad accepted this revision.Sep 1 2021, 8:54 AM

Looks OK to me.

This revision is now accepted and ready to land.Sep 1 2021, 8:54 AM
This revision was landed with ongoing or failed builds.Sep 7 2021, 4:00 AM
This revision was automatically updated to reflect the committed changes.
RKSimon reopened this revision.Jan 23 2022, 12:12 PM

OK, I'll add isGuaranteedNotToBeUndefOrPoison handling

This revision is now accepted and ready to land.Jan 23 2022, 12:12 PM
RKSimon planned changes to this revision.Jan 23 2022, 12:13 PM

OK, I'll add isGuaranteedNotToBeUndefOrPoison handling

Is the "The product of a number with itself is non-negative." case in ValueTracking.cpp's computeKnownBitsMul also broken for the same reason?

OK, I'll add isGuaranteedNotToBeUndefOrPoison handling

Is the "The product of a number with itself is non-negative." case in ValueTracking.cpp's computeKnownBitsMul also broken for the same reason?

Looks OK, Alive2:

----------------------------------------
define i8 @src(i8 %x) {
  %m = mul nsw i8 %x, %x
  %s = lshr i8 %m, 7
  ret i8 %s
}
=>
define i8 @tgt(i8 %x) {
  ret i8 0
}
Transformation seems to be correct!
This revision was not accepted when it landed; it landed in state Changes Planned.Feb 6 2022, 11:40 AM
This revision was automatically updated to reflect the committed changes.