Skip to content

Commit a2a58d9

Browse files
zmodemzmodem
committed
Revert "X86InstrInfo: Support immediates that are +1/-1 different in optimizeCompareInstr"
This casued miscompiles of switches, see comments on the code review. > This extends `optimizeCompareInstr` to re-use previous comparison > results if the previous comparison was with an immediate that was 1 > bigger or smaller. Example: > > CMP x, 13 > ... > CMP x, 12 ; can be removed if we change the SETg > SETg ... ; x > 12 changed to `SETge` (x >= 13) removing CMP > > Motivation: This often happens because SelectionDAG canonicalization > tends to add/subtract 1 often when optimizing for fallthrough blocks. > Example for `x > C` the fallthrough optimization switches true/false > blocks with `!(x > C)` --> `x <= C` and canonicalization turns this into > `x < C + 1`. > > Differential Revision: https://reviews.llvm.org/D110867 This reverts commit e2c7ee0.
1 parent df93c8a commit a2a58d9

File tree

5 files changed

+18
-349
lines changed

5 files changed

+18
-349
lines changed

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

Lines changed: 9 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -4014,8 +4014,8 @@ bool X86InstrInfo::analyzeCompare(const MachineInstr &MI, Register &SrcReg,
40144014
bool X86InstrInfo::isRedundantFlagInstr(const MachineInstr &FlagI,
40154015
Register SrcReg, Register SrcReg2,
40164016
int64_t ImmMask, int64_t ImmValue,
4017-
const MachineInstr &OI, bool *IsSwapped,
4018-
int64_t *ImmDelta) const {
4017+
const MachineInstr &OI,
4018+
bool *IsSwapped) const {
40194019
switch (OI.getOpcode()) {
40204020
case X86::CMP64rr:
40214021
case X86::CMP32rr:
@@ -4066,21 +4066,10 @@ bool X86InstrInfo::isRedundantFlagInstr(const MachineInstr &FlagI,
40664066
int64_t OIMask;
40674067
int64_t OIValue;
40684068
if (analyzeCompare(OI, OISrcReg, OISrcReg2, OIMask, OIValue) &&
4069-
SrcReg == OISrcReg && ImmMask == OIMask) {
4070-
if (OIValue == ImmValue) {
4071-
*ImmDelta = 0;
4072-
return true;
4073-
} else if (static_cast<uint64_t>(ImmValue) ==
4074-
static_cast<uint64_t>(OIValue) - 1) {
4075-
*ImmDelta = -1;
4076-
return true;
4077-
} else if (static_cast<uint64_t>(ImmValue) ==
4078-
static_cast<uint64_t>(OIValue) + 1) {
4079-
*ImmDelta = 1;
4080-
return true;
4081-
} else {
4082-
return false;
4083-
}
4069+
SrcReg == OISrcReg && ImmMask == OIMask && OIValue == ImmValue) {
4070+
assert(SrcReg2 == X86::NoRegister && OISrcReg2 == X86::NoRegister &&
4071+
"should not have 2nd register");
4072+
return true;
40844073
}
40854074
}
40864075
return FlagI.isIdenticalTo(OI);
@@ -4330,7 +4319,6 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
43304319
bool ShouldUpdateCC = false;
43314320
bool IsSwapped = false;
43324321
X86::CondCode NewCC = X86::COND_INVALID;
4333-
int64_t ImmDelta = 0;
43344322

43354323
// Search backward from CmpInstr for the next instruction defining EFLAGS.
43364324
const TargetRegisterInfo *TRI = &getRegisterInfo();
@@ -4377,7 +4365,7 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
43774365
// ... // EFLAGS not changed
43784366
// cmp x, y // <-- can be removed
43794367
if (isRedundantFlagInstr(CmpInstr, SrcReg, SrcReg2, CmpMask, CmpValue,
4380-
Inst, &IsSwapped, &ImmDelta)) {
4368+
Inst, &IsSwapped)) {
43814369
Sub = &Inst;
43824370
break;
43834371
}
@@ -4429,7 +4417,7 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
44294417

44304418
// EFLAGS is used by this instruction.
44314419
X86::CondCode OldCC = X86::COND_INVALID;
4432-
if (MI || IsSwapped || ImmDelta != 0) {
4420+
if (MI || IsSwapped) {
44334421
// We decode the condition code from opcode.
44344422
if (Instr.isBranch())
44354423
OldCC = X86::getCondFromBranch(Instr);
@@ -4482,59 +4470,9 @@ bool X86InstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, Register SrcReg,
44824470
// We swap the condition code and synthesize the new opcode.
44834471
ReplacementCC = getSwappedCondition(OldCC);
44844472
if (ReplacementCC == X86::COND_INVALID) return false;
4485-
ShouldUpdateCC = true;
4486-
} else if (ImmDelta != 0) {
4487-
unsigned BitWidth = TRI->getRegSizeInBits(*MRI->getRegClass(SrcReg));
4488-
// Shift amount for min/max constants to adjust for 8/16/32 instruction
4489-
// sizes.
4490-
switch (OldCC) {
4491-
case X86::COND_L: // x <s (C + 1) --> x <=s C
4492-
if (ImmDelta != 1 || APInt::getSignedMinValue(BitWidth) == CmpValue)
4493-
return false;
4494-
ReplacementCC = X86::COND_LE;
4495-
break;
4496-
case X86::COND_B: // x <u (C + 1) --> x <=u C
4497-
if (ImmDelta != 1 || CmpValue == 0)
4498-
return false;
4499-
ReplacementCC = X86::COND_BE;
4500-
break;
4501-
case X86::COND_GE: // x >=s (C + 1) --> x >s C
4502-
if (ImmDelta != 1 || APInt::getSignedMinValue(BitWidth) == CmpValue)
4503-
return false;
4504-
ReplacementCC = X86::COND_G;
4505-
break;
4506-
case X86::COND_AE: // x >=u (C + 1) --> x >u C
4507-
if (ImmDelta != 1 || CmpValue == 0)
4508-
return false;
4509-
ReplacementCC = X86::COND_A;
4510-
break;
4511-
case X86::COND_G: // x >s (C - 1) --> x >=s C
4512-
if (ImmDelta != -1 || APInt::getSignedMaxValue(BitWidth) == CmpValue)
4513-
return false;
4514-
ReplacementCC = X86::COND_GE;
4515-
break;
4516-
case X86::COND_A: // x >u (C - 1) --> x >=u C
4517-
if (ImmDelta != -1 || APInt::getMaxValue(BitWidth) == CmpValue)
4518-
return false;
4519-
ReplacementCC = X86::COND_AE;
4520-
break;
4521-
case X86::COND_LE: // x <=s (C - 1) --> x <s C
4522-
if (ImmDelta != -1 || APInt::getSignedMaxValue(BitWidth) == CmpValue)
4523-
return false;
4524-
ReplacementCC = X86::COND_L;
4525-
break;
4526-
case X86::COND_BE: // x <=u (C - 1) --> x <u C
4527-
if (ImmDelta != -1 || APInt::getMaxValue(BitWidth) == CmpValue)
4528-
return false;
4529-
ReplacementCC = X86::COND_B;
4530-
break;
4531-
default:
4532-
return false;
4533-
}
4534-
ShouldUpdateCC = true;
45354473
}
45364474

4537-
if (ShouldUpdateCC && ReplacementCC != OldCC) {
4475+
if ((ShouldUpdateCC || IsSwapped) && ReplacementCC != OldCC) {
45384476
// Push the MachineInstr to OpsToUpdate.
45394477
// If it is safe to remove CmpInstr, the condition code of these
45404478
// instructions will be modified.

‎llvm/lib/Target/X86/X86InstrInfo.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -640,8 +640,7 @@ class X86InstrInfo final : public X86GenInstrInfo {
640640
/// CMP %1, %2 and %3 = SUB %2, %1 ; IsSwapped=true
641641
bool isRedundantFlagInstr(const MachineInstr &FlagI, Register SrcReg,
642642
Register SrcReg2, int64_t ImmMask, int64_t ImmValue,
643-
const MachineInstr &OI, bool *IsSwapped,
644-
int64_t *ImmDelta) const;
643+
const MachineInstr &OI, bool *IsSwapped) const;
645644
};
646645

647646
} // namespace llvm

‎llvm/test/CodeGen/X86/optimize-compare.mir

Lines changed: 0 additions & 216 deletions
Original file line numberDiff line numberDiff line change
@@ -379,219 +379,3 @@ body: |
379379
CMP64ri32 %0, 24, implicit-def $eflags
380380
$cl = SETCCr 3, implicit $eflags
381381
...
382-
---
383-
name: opt_redundant_flags_adjusted_imm_0
384-
body: |
385-
bb.0:
386-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_0
387-
; CHECK: [[COPY:%[0-9]+]]:gr64 = COPY $rsi
388-
; CHECK-NEXT: CMP64ri8 [[COPY]], 1, implicit-def $eflags
389-
; CHECK-NEXT: $cl = SETCCr 4, implicit $eflags
390-
; CHECK-NEXT: $bl = SETCCr 15, implicit $eflags
391-
; CHECK-NEXT: $bl = SETCCr 7, implicit $eflags
392-
; CHECK-NEXT: $bl = SETCCr 14, implicit $eflags
393-
; CHECK-NEXT: $bl = SETCCr 6, implicit $eflags
394-
%0:gr64 = COPY $rsi
395-
; CMP+SETCC %0 == 1
396-
CMP64ri8 %0, 1, implicit-def $eflags
397-
$cl = SETCCr 4, implicit $eflags
398-
; CMP+SETCC %0 >= 2; CMP can be removed.
399-
CMP64ri8 %0, 2, implicit-def $eflags
400-
; %0 >=s 2 --> %0 >s 1
401-
$bl = SETCCr 13, implicit $eflags
402-
; %0 >=u 2 --> %0 >u 1
403-
$bl = SETCCr 3, implicit $eflags
404-
; %0 <s 2 --> %0 <=s 1
405-
$bl = SETCCr 12, implicit $eflags
406-
; %0 <u 2 --> %0 <=u 1
407-
$bl = SETCCr 2, implicit $eflags
408-
...
409-
---
410-
name: opt_redundant_flags_adjusted_imm_1
411-
body: |
412-
bb.0:
413-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_1
414-
; CHECK: [[COPY:%[0-9]+]]:gr64 = COPY $rsi
415-
; CHECK-NEXT: CMP64ri8 [[COPY]], 42, implicit-def $eflags
416-
; CHECK-NEXT: $cl = SETCCr 5, implicit $eflags
417-
; CHECK-NEXT: $bl = SETCCr 13, implicit $eflags
418-
; CHECK-NEXT: $bl = SETCCr 3, implicit $eflags
419-
; CHECK-NEXT: $bl = SETCCr 12, implicit $eflags
420-
; CHECK-NEXT: $bl = SETCCr 2, implicit $eflags
421-
%0:gr64 = COPY $rsi
422-
; CMP+SETCC %0 != 42
423-
CMP64ri8 %0, 42, implicit-def $eflags
424-
$cl = SETCCr 5, implicit $eflags
425-
; CMP+SETCC %0 >= 2; CMP can be removed.
426-
CMP64ri8 %0, 41, implicit-def $eflags
427-
; %0 >s 41 --> %0 >=s 42
428-
$bl = SETCCr 15, implicit $eflags
429-
; %0 >u 41 --> %0 >=u 42
430-
$bl = SETCCr 7, implicit $eflags
431-
; %0 <=s 41 --> %0 <s 42
432-
$bl = SETCCr 14, implicit $eflags
433-
; %0 <=u 41 --> %0 <u 42
434-
$bl = SETCCr 6, implicit $eflags
435-
...
436-
---
437-
name: opt_redundant_flags_adjusted_imm_test_cmp
438-
body: |
439-
bb.0:
440-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_test_cmp
441-
; CHECK: [[COPY:%[0-9]+]]:gr8 = COPY $bl
442-
; CHECK-NEXT: TEST8rr [[COPY]], [[COPY]], implicit-def $eflags
443-
; CHECK-NEXT: $cl = SETCCr 14, implicit $eflags
444-
; CHECK-NEXT: $cl = SETCCr 7, implicit $eflags
445-
; CHECK-NEXT: $cl = SETCCr 12, implicit $eflags
446-
%0:gr8 = COPY $bl
447-
TEST8rr %0, %0, implicit-def $eflags
448-
; SET %0 <=s 0
449-
$cl = SETCCr 14, implicit $eflags
450-
; CMP should be removed (%0 >=u 1)
451-
CMP8ri %0, 1, implicit-def $eflags
452-
$cl = SETCCr 3, implicit $eflags
453-
454-
; CMP should be removed (%0 <=s -1)
455-
CMP8ri %0, -1, implicit-def $eflags
456-
$cl = SETCCr 14, implicit $eflags
457-
...
458-
---
459-
name: opt_redundant_flags_adjusted_imm_cmp_test
460-
body: |
461-
bb.0:
462-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_cmp_test
463-
; CHECK: [[COPY:%[0-9]+]]:gr64 = COPY $rsi
464-
; CHECK-NEXT: CMP64ri32 [[COPY]], 1, implicit-def $eflags
465-
; CHECK-NEXT: $cl = SETCCr 13, implicit $eflags
466-
; CHECK-NEXT: [[COPY1:%[0-9]+]]:gr64 = COPY $edi
467-
; CHECK-NEXT: CMP64ri32 [[COPY1]], -1, implicit-def $eflags
468-
; CHECK-NEXT: $cl = SETCCr 14, implicit $eflags
469-
%0:gr64 = COPY $rsi
470-
CMP64ri32 %0, 1, implicit-def $eflags
471-
; TEST should be removed
472-
TEST64rr %0, %0, implicit-def $eflags
473-
$cl = SETCCr 15, implicit $eflags
474-
475-
%1:gr64 = COPY $edi
476-
CMP64ri32 %1, -1, implicit-def $eflags
477-
; TEST should be removed
478-
TEST64rr %1, %1, implicit-def $eflags
479-
$cl = SETCCr 12, implicit $eflags
480-
...
481-
---
482-
name: opt_redundant_flags_adjusted_imm_noopt_0
483-
body: |
484-
bb.0:
485-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_noopt_0
486-
; CHECK: [[COPY:%[0-9]+]]:gr64 = COPY $rsi
487-
; CHECK-NEXT: CMP64ri8 [[COPY]], 42, implicit-def $eflags
488-
; CHECK-NEXT: $cl = SETCCr 4, implicit $eflags
489-
; CHECK-NEXT: CMP64ri8 [[COPY]], 41, implicit-def $eflags
490-
; CHECK-NEXT: $bl = SETCCr 4, implicit $eflags
491-
%0:gr64 = COPY $rsi
492-
; CMP+SETCC %0 <s 1
493-
CMP64ri8 %0, 42, implicit-def $eflags
494-
$cl = SETCCr 4, implicit $eflags
495-
; CMP should not be removed.
496-
CMP64ri8 %0, 41, implicit-def $eflags
497-
; %0 == 41
498-
$bl = SETCCr 4, implicit $eflags
499-
...
500-
---
501-
name: opt_redundant_flags_adjusted_imm_noopt_1
502-
body: |
503-
bb.0:
504-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_noopt_1
505-
; CHECK: [[COPY:%[0-9]+]]:gr32 = COPY $esi
506-
; CHECK-NEXT: CMP32ri [[COPY]], 2147483647, implicit-def $eflags
507-
; CHECK-NEXT: CMP32ri [[COPY]], -2147483648, implicit-def $eflags
508-
; CHECK-NEXT: $bl = SETCCr 12, implicit $eflags
509-
; CHECK-NEXT: CMP32ri [[COPY]], 4294967295, implicit-def $eflags
510-
; CHECK-NEXT: CMP32ri [[COPY]], -2147483648, implicit-def $eflags
511-
; CHECK-NEXT: $bl = SETCCr 12, implicit $eflags
512-
; CHECK-NEXT: CMP32ri [[COPY]], 2147483647, implicit-def $eflags
513-
; CHECK-NEXT: CMP32ri [[COPY]], -2147483648, implicit-def $eflags
514-
; CHECK-NEXT: $bl = SETCCr 13, implicit $eflags
515-
; CHECK-NEXT: CMP32ri [[COPY]], 4294967295, implicit-def $eflags
516-
; CHECK-NEXT: CMP32ri [[COPY]], 0, implicit-def $eflags
517-
; CHECK-NEXT: $bl = SETCCr 2, implicit $eflags
518-
; CHECK-NEXT: CMP32ri [[COPY]], 4294967295, implicit-def $eflags
519-
; CHECK-NEXT: CMP32ri [[COPY]], 0, implicit-def $eflags
520-
; CHECK-NEXT: $bl = SETCCr 3, implicit $eflags
521-
%0:gr32 = COPY $esi
522-
; CMP+SETCC %0 == INT32_MAX
523-
CMP32ri %0, 2147483647, implicit-def $eflags
524-
; CMP should not be removed.
525-
CMP32ri %0, -2147483648, implicit-def $eflags
526-
; %0 <s INT32_MIN
527-
$bl = SETCCr 12, implicit $eflags
528-
529-
CMP32ri %0, 4294967295, implicit-def $eflags
530-
; CMP should not be removed.
531-
CMP32ri %0, -2147483648, implicit-def $eflags
532-
$bl = SETCCr 12, implicit $eflags
533-
534-
CMP32ri %0, 2147483647, implicit-def $eflags
535-
; CMP should not be removed.
536-
CMP32ri %0, -2147483648, implicit-def $eflags
537-
$bl = SETCCr 13, implicit $eflags
538-
539-
CMP32ri %0, 4294967295, implicit-def $eflags
540-
; should not be removed
541-
CMP32ri %0, 0, implicit-def $eflags
542-
$bl = SETCCr 2, implicit $eflags
543-
544-
CMP32ri %0, 4294967295, implicit-def $eflags
545-
; should not be removed
546-
CMP32ri %0, 0, implicit-def $eflags
547-
$bl = SETCCr 3, implicit $eflags
548-
...
549-
---
550-
name: opt_redundant_flags_adjusted_imm_noopt_2
551-
body: |
552-
bb.0:
553-
; CHECK-LABEL: name: opt_redundant_flags_adjusted_imm_noopt_2
554-
; CHECK: [[COPY:%[0-9]+]]:gr16 = COPY $cx
555-
; CHECK-NEXT: CMP16ri [[COPY]], -32768, implicit-def $eflags
556-
; CHECK-NEXT: CMP16ri [[COPY]], 32767, implicit-def $eflags
557-
; CHECK-NEXT: $bl = SETCCr 15, implicit $eflags
558-
; CHECK-NEXT: CMP16ri [[COPY]], 65535, implicit-def $eflags
559-
; CHECK-NEXT: CMP16ri [[COPY]], 32767, implicit-def $eflags
560-
; CHECK-NEXT: $bl = SETCCr 15, implicit $eflags
561-
; CHECK-NEXT: CMP16ri [[COPY]], -32768, implicit-def $eflags
562-
; CHECK-NEXT: CMP16ri [[COPY]], 32767, implicit-def $eflags
563-
; CHECK-NEXT: $bl = SETCCr 14, implicit $eflags
564-
; CHECK-NEXT: CMP16ri [[COPY]], 0, implicit-def $eflags
565-
; CHECK-NEXT: CMP16ri [[COPY]], 65535, implicit-def $eflags
566-
; CHECK-NEXT: $bl = SETCCr 4, implicit $eflags
567-
; CHECK-NEXT: CMP16ri [[COPY]], 0, implicit-def $eflags
568-
; CHECK-NEXT: CMP16ri [[COPY]], 65535, implicit-def $eflags
569-
; CHECK-NEXT: $bl = SETCCr 6, implicit $eflags
570-
%0:gr16 = COPY $cx
571-
; CMP+SETCC %0 == INT16_MIN
572-
CMP16ri %0, -32768, implicit-def $eflags
573-
; CMP should not be removed.
574-
CMP16ri %0, 32767, implicit-def $eflags
575-
; %0 >s INT16_MAX
576-
$bl = SETCCr 15, implicit $eflags
577-
578-
CMP16ri %0, 65535, implicit-def $eflags
579-
; CMP should not be removed.
580-
CMP16ri %0, 32767, implicit-def $eflags
581-
$bl = SETCCr 15, implicit $eflags
582-
583-
CMP16ri %0, -32768, implicit-def $eflags
584-
; CMP should not be removed.
585-
CMP16ri %0, 32767, implicit-def $eflags
586-
$bl = SETCCr 14, implicit $eflags
587-
588-
CMP16ri %0, 0, implicit-def $eflags
589-
; should not be removed
590-
CMP16ri %0, 65535, implicit-def $eflags
591-
$bl = SETCCr 4, implicit $eflags
592-
593-
CMP16ri %0, 0, implicit-def $eflags
594-
; should not be removed
595-
CMP16ri %0, 65535, implicit-def $eflags
596-
$bl = SETCCr 6, implicit $eflags
597-
...

0 commit comments

Comments
 (0)