This is an archive of the discontinued LLVM Phabricator instance.

[X86] Invert transforming `(x * (Pow2_Ceil(C1) - (1 << C0))) & C1` -> `(-x << C0) & C1`
ClosedPublic

Authored by goldstein.w.n on May 10 2023, 1:10 PM.

Details

Summary

We can detect the case under the following circumstances:
Take (Pow2_Ceil(C1) - (1 << C0)) as C2.

  1. C2 is NOT a power of 2.
  2. C2 + LeastSignificantBit(C2) is a nonzero power of 2.
  3. C2 u>= C1

The motivation is the middle end transforms:

`(-x << C0) & C1`

to

`(x * (Pow2_Ceil(C1) - (1 << C2))) & C1`

As it saves IR instructions. On X86 the two instruction, sub and
shl, and better than the mul so we want to undo the transform.

This comes up when shifting a bit-mask by a byte-misalignment i.e:

`y << ((-(uintptr)x * 8) & 63)`

Alive2 Proofs (including all cases with undefs in the vector):
https://alive2.llvm.org/ce/z/f-65b6

Diff Detail

Event Timeline

goldstein.w.n created this revision.May 10 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 1:10 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.May 10 2023, 1:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2023, 1:10 PM
goldstein.w.n retitled this revision from [X86] Transforming `(x*C0)&C1`->`(-x<<log2(C0&(-C0)))&C1`; NFC to [X86] Transforming `(x*C0)&C1`->`(-x<<log2(C0&(-C0)))&C1`.

The title is misleading because we cannot transforming arbitrary C0. Maybe "[X86] Invert transforming (x * (Pow2_Ceil(C0) - (1 << C1))) & C0 -> (-x << C1) & C0"?

Typo: "wan't" -> "want"

llvm/lib/Target/X86/X86ISelLowering.cpp
50299–50301

The comments don't match description. Maybe change one side.

goldstein.w.n added inline comments.May 11 2023, 8:20 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
50299–50301

hmm? Think its correct although it may be confusing because its talking about the transform we are undoing, not what we are doing.

pengfei added inline comments.May 11 2023, 8:26 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
50299–50301

Just nitpick. I mean you use C1/C2 in description but C0/C1 here. Also and/mul vs. &/*.

goldstein.w.n retitled this revision from [X86] Transforming `(x*C0)&C1`->`(-x<<log2(C0&(-C0)))&C1` to [X86] Invert transforming `(x * (Pow2_Ceil(C1) - (1 << C0))) & C1` -> `(-x << C0) & C1`.May 11 2023, 3:02 PM
goldstein.w.n edited the summary of this revision. (Show Details)
goldstein.w.n edited the summary of this revision. (Show Details)

Clarify comments / commit message

llvm/lib/Target/X86/X86ISelLowering.cpp
50299–50301

Ahh I see. Did the &/* in the title to save characters. Then finished the summary like that to stay consistent.

But done, made this part consistent as well.

goldstein.w.n marked 2 inline comments as done.May 11 2023, 3:10 PM
pengfei accepted this revision.May 11 2023, 7:23 PM

Thanks! LGTM.

This revision is now accepted and ready to land.May 11 2023, 7:23 PM
RKSimon accepted this revision.May 13 2023, 4:19 AM

LGTM with a few minors

llvm/lib/Target/X86/X86ISelLowering.cpp
50332

There's no strong reason to require N1 to be a splat - you should be able to use matchUnaryPredicate, this is true for N01C as well, but non-uniform shift costs are trickier (even where they're legal).

But all this can be looked at in a followup - maybe add a TODO?

50339

N0.getOperand(0)

50341

assert message

goldstein.w.n marked 3 inline comments as done.May 13 2023, 10:12 AM
goldstein.w.n added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
50332

Added TODO (tests also have todo for this).

goldstein.w.n marked an inline comment as done.

Fix some nits

This revision was landed with ongoing or failed builds.May 13 2023, 12:36 PM
This revision was automatically updated to reflect the committed changes.