This is an archive of the discontinued LLVM Phabricator instance.

[X86] Search through associative operators for BMI patterns (BLSI, BLSR, BLSMSK)
ClosedPublic

Authored by goldstein.w.n on Jan 6 2023, 7:40 PM.

Details

Summary

(a & (-b)) & b is often lowered as:

%sub  = sub i32     0, %b
%and0 = and i32  %sub, %a
%and1 = and i32 %and0, %b

Which won't get detected by the BLSI pattern as b & -b are never in
the same SDNode.

This patch will do a small search through associative operators and try
and place BMI patterns in the same node so they will hit the pattern.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 6 2023, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 7:40 PM
goldstein.w.n requested review of this revision.Jan 6 2023, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 7:40 PM

Isn’t the property “associative” rather than “commutative”?

craig.topper added inline comments.Jan 6 2023, 9:41 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
48801

Do not use all caps for OPC

48808

If you only look through binops without going through bitcasts, extends, or truncates, you can assume the types will always match.

48828

You can use isNullConstant(Op.getOperand(0) here.

48837

isOneConstant(Op.getOperand(1))

48846

isAllOnesConstant(Op.getOperand(OpIdx)

48854

Lower case start of function name

48858

Just check VT != MVT::i32 && VT != MVT::i64 unless you're trying to support i33-i63?

48865

You can drop all the curly braces here.

49091

Space after if

49092

Over indented

52008

Space after if

52009

Over indented

goldstein.w.n marked 12 inline comments as done.

Fix format issues.
Remove unneeded code.
Cleanup code.

goldstein.w.n added a comment.EditedJan 7 2023, 10:03 AM

Change commutable -> assosative in commit / code.

goldstein.w.n retitled this revision from [X86]] Search through commutable operators for BMI patterns (BLSI, BLSR, BLSMSK) to [X86] Search through associative operators for BMI patterns (BLSI, BLSR, BLSMSK).Jan 7 2023, 10:04 AM
goldstein.w.n edited the summary of this revision. (Show Details)

Remove unneeded braces in a few more places

pengfei added inline comments.Jan 31 2023, 11:49 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
48807

Add period in the end. The same for others below.

48811

SDLoc DL(Op);

48841

Can we assume it is canonical and just check operand 1?

goldstein.w.n marked 3 inline comments as done.Feb 1 2023, 5:40 PM

Fix nits + rebase

pengfei accepted this revision.Feb 1 2023, 6:56 PM

LGTM.

This revision is now accepted and ready to land.Feb 1 2023, 6:56 PM

@pengfei I was thinking patch, is it liable to cause an infinite loop? Its just reordering and another place might undo its changes. Maybe it makes more sense in X86::ISelDAGToDag?

This revision was landed with ongoing or failed builds.Feb 6 2023, 12:16 PM
This revision was automatically updated to reflect the committed changes.