This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Combine (X op Y) == X --> Y == 0
ClosedPublic

Authored by paquette on Oct 6 2022, 11:13 AM.

Details

Summary

This matches patterns of the form

(X op Y) == X

And transforms them to

Y == 0

where appropriate.

Example: https://godbolt.org/z/hfW811c7W

Diff Detail

Event Timeline

paquette created this revision.Oct 6 2022, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 11:13 AM
paquette requested review of this revision.Oct 6 2022, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 11:13 AM
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Oct 6 2022, 12:49 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6075

Can you really get away without checking the mi_match result?

6087–6088

Should there be commutative variants of the matchers like in the IR version?

paquette added inline comments.Oct 6 2022, 1:05 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6087–6088

Yeah, something for G_ICMP on eq/ne would be useful. The binops already commute.

tschuett added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6075

There is no [[nodiscard]] on mi_match?

paquette added inline comments.Oct 6 2022, 1:57 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6075

I didn't get a warning, but maybe other compilers would warn?

(I should add an assert that MI is a G_ICMP at the top though)

paquette added inline comments.Oct 6 2022, 2:08 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6087–6088

I think for a commutative matcher to make an impact here we'd need to be able to write like

if (!mi_match(Dst, MRI,
              m_GICmp(m_Pred(Pred), m_Reg(X),
                      m_any_of(m_GAdd(m_SpecificReg(X), m_Reg(Y)),
                               m_GSub(m_SpecificReg(X), m_Reg(Y)),
                               m_GXor(m_SpecificReg(X), m_Reg(Y))))) ||
    !ICmpInst::isEquality(Pred))
  return false;

But I don't think that it's guaranteed that m_Reg(X) will be evaluated before m_SpecificReg(X)?

paquette updated this revision to Diff 465939.Oct 6 2022, 5:24 PM

Update to use D135415

arsenm added inline comments.Oct 7 2022, 7:12 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6083

Not using the _c_ versions?

paquette updated this revision to Diff 466241.Oct 7 2022, 8:23 PM

Use shiny new commutative matcher

arsenm added inline comments.Oct 7 2022, 9:11 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6079

I'm not seeing why sub needs to be handled as a separate case

paquette added inline comments.Oct 7 2022, 9:18 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
6079

The Y = X == OpLHS ? OpRHS : X == OpRHS ? OpLHS : Register(); part would be wrong for sub because sub is noncommutative.

arsenm accepted this revision.Oct 7 2022, 9:54 PM
This revision is now accepted and ready to land.Oct 7 2022, 9:54 PM
This revision was landed with ongoing or failed builds.Oct 11 2022, 9:53 AM
This revision was automatically updated to reflect the committed changes.