This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by lebedev.ri on Jun 1 2019, 4:28 PM.

Details

Summary

Finally tying up loose ends here.

The problem is quite simple:
If we have pattern (x >> start) & (1 << nbits) - 1,
and then truncate the result, that truncation will be propagated upwards,
into the and. And that isn't currently handled.

I'm only fixing pattern a here,
the same fix will be needed for patterns b/c too.

I *think* this isn't missing any extra legality checks,
since we only look past truncations. Similary, i don't think
we can get any other truncation there other than i64->i32.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 1 2019, 4:28 PM
lebedev.ri updated this revision to Diff 202588.Jun 2 2019, 1:22 AM

Don't peek through truncation of 'X' if we have BMI2.

RKSimon added inline comments.Jun 2 2019, 4:50 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
2997 ↗(On Diff #202588)

assertion messages:

assert(V.getSimpleValueType() == MVT::i32 &&
           V.getOperand(0).getSimpleValueType() == MVT::i64 &&
           "Expected i64 -> i32 truncation");
3149 ↗(On Diff #202588)

Again, you can probably merge these asserts

lebedev.ri updated this revision to Diff 202597.Jun 2 2019, 5:45 AM
lebedev.ri marked 2 inline comments as done.

Cleanup diff.

Rebase, mostly NFC.

lebedev.ri updated this revision to Diff 202670.Jun 3 2019, 2:37 AM

Rebased, NFC.

lebedev.ri marked an inline comment as done.Jun 3 2019, 8:45 AM
lebedev.ri added inline comments.
test/CodeGen/X86/extract-lowbits.ll
22–23 ↗(On Diff #202670)

Thinking about it, i guess at least 2 more patterns are missing:

"get me all bits except y high bits"
;   c') x &  (-1 >> (y))
;   d') x << (y) >> (y)

... to get the 'num bits to extract' we'd need to also insert sub 32 - y.

I don't think we can just select these two patterns, and not look for (32 - y),
because we are so late that no peephole pass will cleanup (32 - (32 - y)) into y.
(That i know of?)

Similarly, i don't *think* we can instcombine these into any other pattern.

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

This revision is now accepted and ready to land.Jun 25 2019, 4:20 PM

Thank you for the review!