Page MenuHomePhabricator

[X86] X86DAGToDAGISel::matchBitExtract(): pattern c: truncation awareness
ClosedPublic

Authored by lebedev.ri on Jun 3 2019, 2:37 AM.

Details

Summary

The one thing of note here is that the 'bitwidth' constant (32/64) was previously pessimistic.
Given x & (-1 >> (C - z)), we were taking C to be bitwidth(x), but in reality
we want (-1 >> (C - z)) pattern to mean "low z bits must be all-ones".
And for that, C should be bitwidth(-1 >> (C - z)), i.e. of the shift operation itself.

Last pattern D does not seem to exhibit any of these truncation issues.
Although it has the opposite problem - if we extract low bits (no shift) from i64,
and then truncate to i32, then we fail to shrink this 64-bit extraction into 32-bit extraction.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri marked an inline comment as done.Jun 3 2019, 2:58 AM
lebedev.ri added inline comments.
test/CodeGen/X86/extract-bits.ll
6593 ↗(On Diff #202672)

Ah no, this is wrong.

lebedev.ri updated this revision to Diff 202678.Jun 3 2019, 3:00 AM

Only match truly all-ones constants.

RKSimon added inline comments.Jun 3 2019, 4:01 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
3050 ↗(On Diff #202678)

Inconsistent - checkOneUse or &checkOneUse ?

lebedev.ri added inline comments.Jun 3 2019, 4:14 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
3050 ↗(On Diff #202678)

Hm, indeed. I guess since no one complained thus far both variants are ok.
This problem is all over this function, so i'd rather fix it as a follow-up,
but i'm not sure in which direction it should be fixed.

Ping. Would it be better to go for post-commit review mode for these X86DAGToDAGISel::matchBitExtract() changes?

craig.topper added inline comments.Jun 25 2019, 4:25 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
3070 ↗(On Diff #202678)

The isAllOnes capture isn't used.

lebedev.ri marked 2 inline comments as done.

Addressed review nit.

RKSimon accepted this revision.Jun 26 2019, 4:34 AM

LGTM

lib/Target/X86/X86ISelDAGToDAG.cpp
3271 ↗(On Diff #206601)

should this still be here?

This revision is now accepted and ready to land.Jun 26 2019, 4:34 AM

Thank you for the review!

lib/Target/X86/X86ISelDAGToDAG.cpp
3271 ↗(On Diff #206601)

I think not :)

3070 ↗(On Diff #202678)

Hmm, indeed, thanks!

This revision was automatically updated to reflect the committed changes.