Page MenuHomePhabricator

X86InstrInfo: Optimize more combinations of SUB+CMP
ClosedPublic

Authored by MatzeB on Sep 30 2021, 11:10 AM.

Details

Summary

X86InstrInfo::optimizeCompareInstr would only optimize a SUB followed by a CMP in isRedundantFlagInstr. This extends the code to also look for other combinations like CMP+CMP, TEST+TEST, SUB x,0+TEST.

  • Change isRedundantFlagInstr to run analyzeCompareInstr on the candidate instruction and compare the results. This normalizes things and gives consistent results for various comparisons (CMP x, y, SUB x, y) and immediate cases (TEST x, x, SUB x, 0, CMP x, 0...).
  • Turn isRedundantFlagInstr into a member function so it can call analyzeCompare.
  • We now also check isRedundantFlagInstr even if IsCmpZero is true, since we now have cases like TEST+TEST.

Diff Detail

Event Timeline

MatzeB created this revision.Sep 30 2021, 11:10 AM
MatzeB requested review of this revision.Sep 30 2021, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2021, 11:10 AM
MatzeB edited the summary of this revision. (Show Details)Sep 30 2021, 11:28 AM
foad added a subscriber: foad.Oct 1 2021, 6:21 AM
pengfei added inline comments.Oct 1 2021, 7:09 AM
llvm/test/CodeGen/X86/optimize-compare.mir
221

Do we have such CMP + CMP cases in reality? I think optimization should merged them already. The only case I can think is eflags are clobbered, so that we have to def them again which can't be optimizated here either.

Do we have such CMP + CMP cases in reality? I think optimization should merged them already. The only case I can think is eflags are clobbered, so that we have to def them again which can't be optimizated here either.

Yes we do:

  • If they are in different basic blocks, then SelectionDAG won't merge them.
  • SelectionDAG has a tendency to canonicalize into different directions. My motivating sourcecode here says if (x == 1) { ... } else if (x > 1) { ... } but SeletionDAG normalizes that to x == 1 and x >= 2 so generic mechanisms like MachineCSE are not catching it.
  • I've also seen several examples now, where we middleend canonicalization for i1 values leads to strange outcomes. You can for example look at https://godbolt.org/z/1T63zKqvc and see how the same comparison is repeated there (yes we should rather fix the root cause for that, but for now it's one more example of duplicated comparisons).

Do we have such CMP + CMP cases in reality? I think optimization should merged them already. The only case I can think is eflags are clobbered, so that we have to def them again which can't be optimizated here either.

Yes we do:

  • If they are in different basic blocks, then SelectionDAG won't merge them.
  • SelectionDAG has a tendency to canonicalize into different directions. My motivating sourcecode here says if (x == 1) { ... } else if (x > 1) { ... } but SeletionDAG normalizes that to x == 1 and x >= 2 so generic mechanisms like MachineCSE are not catching it.
  • I've also seen several examples now, where we middleend canonicalization for i1 values leads to strange outcomes. You can for example look at https://godbolt.org/z/1T63zKqvc and see how the same comparison is repeated there (yes we should rather fix the root cause for that, but for now it's one more example of duplicated comparisons).

Makes sense to me. Thank you!

RKSimon added inline comments.Oct 23 2021, 2:07 AM
llvm/test/CodeGen/X86/optimize-compare.mir
208

Pre-commit these and rebase so the patch show the codegen diff?

RKSimon added inline comments.Oct 23 2021, 5:47 AM
llvm/lib/Target/X86/X86InstrInfo.h
634

SrcRegs -> SrcReg2 ?

635

Please describe IsSwapped as well

MatzeB updated this revision to Diff 381810.Oct 24 2021, 5:02 PM
  • Rebased on precommitted tests.
  • Rewrite docu comment for isRedundantCompareInstr.
MatzeB marked 2 inline comments as done.Oct 24 2021, 5:03 PM
MatzeB added inline comments.
llvm/lib/Target/X86/X86InstrInfo.h
634

I previously just moved this text from the .cpp file...

I'll rewrite the whole comment from scratch.

MatzeB updated this revision to Diff 381811.Oct 24 2021, 5:09 PM
MatzeB marked an inline comment as done.

Document new SrcReg2.isPhysical() test. (The test fixes a pre-existing bug though likely benign since nothing should actually use physical registers on CMP/SUB/TEST instructions that early in the codegen pipeline)

SGTM, does anyone else have any comments?

llvm/lib/Target/X86/X86InstrInfo.cpp
4330

worth moving this up, just after the switch()?

4497–4498

assert message?

MatzeB updated this revision to Diff 382009.Oct 25 2021, 8:28 AM
MatzeB marked 3 inline comments as done.
MatzeB marked an inline comment as done.
RKSimon accepted this revision.Oct 28 2021, 4:14 AM

LGTM

This revision is now accepted and ready to land.Oct 28 2021, 4:14 AM
This revision was landed with ongoing or failed builds.Oct 28 2021, 10:34 AM
This revision was automatically updated to reflect the committed changes.