This is an archive of the discontinued LLVM Phabricator instance.

[X86] X86DAGToDAGISel: handle BZHI selection too, not just BEXTR.
ClosedPublic

Authored by lebedev.ri on Oct 11 2018, 1:08 PM.

Details

Summary

As discussed in D52304 / IRC, we now have pattern matching for
'bit extract' in two places - tablegen and X86DAGToDAGISel.
There are 4 patterns.
And we will have a problem with x & (-1 >> (32 - y)) pattern.

  • If the mask is one-use, then it is always unfolded into x << (32 - y) >> (32 - y) first. Thus, the existing test coverage is already broken.
  • If it is not one-use, then it is not unfolded, and is matched as BZHI.
  • If it is not one-use, we will not match it as BEXTR. And if it is one-use, it will have been unfolded already.

So we will either not handle that pattern for BEXTR, or not have test coverage for it.
This is bad.

As discussed with @craig.topper, let's unify this matching, and do everything in X86DAGToDAGISel.
Then we will not have code duplication, and will have proper test coverage.

This indeed does not affect any tests, and this is great.
It means that for these two patterns, the X86DAGToDAGISel is identical to the tablegen version.

Please review carefully, i'm not fully sure about that intrinsic change, and introduction of the new X86ISD opcode.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper added inline comments.Oct 14 2018, 11:18 AM
lib/Target/X86/X86ISelDAGToDAG.cpp
2659

return CanHaveExtraUses || Op.hasOneUse();

lebedev.ri marked an inline comment as done.

Simplify checkOneUse().

This revision is now accepted and ready to land.Oct 15 2018, 10:01 PM

LGTM

Thank you for the review!
Note that this depends on D52348.

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.