This is an archive of the discontinued LLVM Phabricator instance.

[X86] X86DAGToDAGISel::matchBitExtract(): support 'num high bits to clear' pattern
ClosedPublic

Authored by lebedev.ri on Aug 11 2021, 12:48 PM.

Details

Summary

Currently, we only deal with the case where we can match
the number of low bits to be kept, i.e.:

x & ((1 << y) - 1)

will extract low y bits of x.

But what will

x & (-1 >> y)

do?

Logically, it will extract bitwidth(x) - y low bits, i.e.:

x & ~(-1 << (bitwidth(x)-y))

... except we can't do such a transformation in IR in general,
because if we wanted to extract all the bits (-1 >> 0) is fine,
but -1 << bitwidth(x) would be poison: https://alive2.llvm.org/ce/z/BKJZfw,

Yet, here with BMI's BEXTR and BMI2's BZHI we don't have any such problems with edge-cases.
So what we can do is: https://alive2.llvm.org/ce/z/gm5M2B

As briefly discussed with @craig.topper, this appears to be not worse than what we'd end up with currently (a pair of shifts):

Diff Detail

Event Timeline

lebedev.ri created this revision.Aug 11 2021, 12:48 PM
lebedev.ri requested review of this revision.Aug 11 2021, 12:48 PM

That being said, i guess we should restrict this to BMI2 BZHI only:
https://godbolt.org/z/zxhPa8Gdh https://godbolt.org/z/KhWhE1eeE
Let me do that..

Restrict to BMI2 BZHI.

RKSimon added inline comments.Aug 25 2021, 7:58 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3473

is this a match function anymore? should it canonicalizeShiftAmt?

lebedev.ri marked an inline comment as done.

@RKSimon thank you for taking a look!
I guess you're right, renamed lambda accordingly.

craig.topper added inline comments.Sep 7 2021, 11:37 AM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3594

NVT is a scalar isn't it? So this could just be getSizeINBits()?

llvm/test/CodeGen/X86/clear-highbits.ll
929

This doesn't seem better. But maybe it increases parallelism the if the shrxl use wasn't a call?

lebedev.ri updated this revision to Diff 371169.Sep 7 2021, 1:25 PM
lebedev.ri marked an inline comment as done.

Thanks for taking a look!

Yeah, i keep wondering about these one-use checks.
I'm not sure what's right.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3594

Err, right.

lebedev.ri updated this revision to Diff 371185.Sep 7 2021, 2:14 PM

Fix one-use checks - this is hilariously hideous.

lebedev.ri marked an inline comment as done.Sep 7 2021, 2:15 PM
craig.topper added inline comments.Sep 7 2021, 9:13 PM
llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3401

Can we use Optional::getValueOr here?

lebedev.ri updated this revision to Diff 371280.Sep 8 2021, 1:57 AM
lebedev.ri marked an inline comment as done.

Thank you for taking a look!
Use Optional::getValueOr().

This revision is now accepted and ready to land.Sep 8 2021, 9:14 AM

LGTM

Thank you for the review!

This revision was landed with ongoing or failed builds.Sep 8 2021, 9:27 AM
This revision was automatically updated to reflect the committed changes.