This is an archive of the discontinued LLVM Phabricator instance.

[Instcombine] fold logic ops to select
ClosedPublic

Authored by bcl5980 on Dec 1 2022, 12:20 AM.

Diff Detail

Event Timeline

bcl5980 created this revision.Dec 1 2022, 12:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 12:20 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bcl5980 requested review of this revision.Dec 1 2022, 12:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 12:20 AM
bcl5980 updated this revision to Diff 479971.Dec 4 2022, 8:54 PM

code rebase

bcl5980 updated this revision to Diff 480118.Dec 5 2022, 8:26 AM

rebase code after precommit

RKSimon added inline comments.Dec 8 2022, 10:02 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2603–2605

Rename AisB ? It seems to suggest that A is known to be equivalent to B.

llvm/lib/Transforms/InstCombine/InstCombineInternal.h
374–376

Explain the purposes of NeedNotD and AisB

bcl5980 updated this revision to Diff 481515.Dec 8 2022, 9:47 PM

address comments.

spatel added inline comments.Jan 3 2023, 11:53 AM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
3101–3102

This is more general than needed. Complexity canonicalization guarantees that the 'not' is operand 1.

We could also check for the common operand out here for efficiency?

if (match(Op0, m_And(m_Value(A), m_Value(C))) &&
    match(Op1, m_Not(m_Or(m_Value(B), m_Value(D)))) &&
    hasCommonOperand(A, C, B, D) && (Op0->hasOneUse() || Op1->hasOneUse())) {
bcl5980 added inline comments.Jan 3 2023, 7:08 PM
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
3101–3102

If we use hasCommonOperand we needn't call matchSelectFromAndOr I think.
matchSelectFromAndOr will peek through the bitcast. And getSelectCondition consider more about the pattern with constant. For now I haven't add the support of these complex patterns. But maybe we can add them in the future. So I prefer to reuse the current code.

bcl5980 updated this revision to Diff 486185.Jan 4 2023, 12:35 AM

rebase code

spatel accepted this revision.Jan 4 2023, 7:25 AM

LGTM - see inline comment for a minor adjustment.

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
2690

The name "SelectNotFalseVal" is confusing to me. I would prefer to name this "InvertFalseVal". Then adjust the code comment above to say:

/// When InvertFalseVal is set to true, we try to match the pattern
/// where we have peeked through a 'not' op and A and B are the same:
/// (A & C) | ~(A | D) --> (A & C) | (~A & ~D) --> A' ? C : ~D
This revision is now accepted and ready to land.Jan 4 2023, 7:25 AM
bcl5980 updated this revision to Diff 486447.Jan 4 2023, 7:04 PM

address comments.

This revision was landed with ongoing or failed builds.Jan 4 2023, 8:04 PM
This revision was automatically updated to reflect the committed changes.