Page MenuHomePhabricator

X86InstrInfo: Optimize more combinations of SUB+CMP
Needs ReviewPublic

Authored by MatzeB on Thu, Sep 30, 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.Thu, Sep 30, 11:10 AM
MatzeB requested review of this revision.Thu, Sep 30, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 30, 11:10 AM
MatzeB edited the summary of this revision. (Show Details)Thu, Sep 30, 11:28 AM
foad added a subscriber: foad.Fri, Oct 1, 6:21 AM
pengfei added inline comments.Fri, Oct 1, 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!