Skip to content

Commit e1c3538

Browse files
swiftixswiftix
committed
redundundant-overflow-check-removal improvements
- Code-refactoring - Support for comparison followed by a cond_fail - Correctness fixes for relation propagation
1 parent 2326cb7 commit e1c3538

File tree

2 files changed

+177
-97
lines changed

2 files changed

+177
-97
lines changed

‎lib/SILOptimizer/Transforms/RedundantOverflowCheckRemoval.cpp

Lines changed: 144 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ class RedundantOverflowCheckRemovalPass : public SILFunctionTransform {
3737
RedundantOverflowCheckRemovalPass() {}
3838

3939
/// This enum represents a relationship between two operands.
40-
/// The relationship represented by arithmetic operators represent the
40+
/// The relationship represented by arithmetic operators represents the
4141
/// information that the operation did not trap.
4242
///
4343
/// The following code translate (with the correct signedness prefix):
@@ -57,9 +57,11 @@ class RedundantOverflowCheckRemovalPass : public SILFunctionTransform {
5757
/// the function.
5858
struct Constraint {
5959
Constraint(SILBasicBlock *BB, SILValue L, SILValue R, ValueRelation Rel) :
60-
DominatingBlock(BB), Left(L), Right(R), Relationship(Rel) {}
60+
DominatingBlock(BB), Left(L), Right(R), Relationship(Rel) {
61+
DEBUG(dump());
62+
}
6163

62-
/// The constraint is valid blocks dominated by this block.
64+
/// The constraint is valid in blocks dominated by this block.
6365
SILBasicBlock *DominatingBlock;
6466
/// The first operand.
6567
SILValue Left;
@@ -515,45 +517,151 @@ class RedundantOverflowCheckRemovalPass : public SILFunctionTransform {
515517
return false;
516518
}
517519

520+
Optional<ValueRelation> getArithOpRelation(BuiltinInst *BI) {
521+
ValueRelation Rel;
522+
switch (BI->getBuiltinInfo().ID) {
523+
default:
524+
return None;
525+
case BuiltinValueKind::SAddOver:
526+
Rel = ValueRelation::SAdd;
527+
break;
528+
case BuiltinValueKind::UAddOver:
529+
Rel = ValueRelation::UAdd;
530+
break;
531+
case BuiltinValueKind::SSubOver:
532+
Rel = ValueRelation::SSub;
533+
break;
534+
case BuiltinValueKind::USubOver:
535+
Rel = ValueRelation::USub;
536+
break;
537+
case BuiltinValueKind::SMulOver:
538+
Rel = ValueRelation::SMul;
539+
break;
540+
case BuiltinValueKind::UMulOver:
541+
Rel = ValueRelation::UMul;
542+
break;
543+
}
544+
return Rel;
545+
}
546+
547+
void addComparisonRelation(BuiltinInst *CMP, SILBasicBlock *TrueBB,
548+
SILBasicBlock *FalseBB) {
549+
// The relationship expressed in the builtin.
550+
ValueRelation TrueRel;
551+
ValueRelation FalseRel;
552+
bool Swap = false;
553+
554+
switch (CMP->getBuiltinInfo().ID) {
555+
default:
556+
return;
557+
case BuiltinValueKind::ICMP_NE: {
558+
SILValue Left = CMP->getOperand(0);
559+
SILValue Right = CMP->getOperand(1);
560+
if (FalseBB)
561+
Constraints.push_back(
562+
Constraint(FalseBB, Left, Right, ValueRelation::EQ));
563+
return;
564+
}
565+
case BuiltinValueKind::ICMP_EQ: {
566+
SILValue Left = CMP->getOperand(0);
567+
SILValue Right = CMP->getOperand(1);
568+
if (TrueBB)
569+
Constraints.push_back(
570+
Constraint(TrueBB, Left, Right, ValueRelation::EQ));
571+
return;
572+
}
573+
case BuiltinValueKind::ICMP_SLE:
574+
TrueRel = ValueRelation::SLE;
575+
FalseRel = ValueRelation::SLT;
576+
break;
577+
case BuiltinValueKind::ICMP_SLT:
578+
TrueRel = ValueRelation::SLT;
579+
FalseRel = ValueRelation::SLE;
580+
break;
581+
case BuiltinValueKind::ICMP_SGE:
582+
TrueRel = ValueRelation::SLT;
583+
FalseRel = ValueRelation::SLE;
584+
Swap = true;
585+
break;
586+
case BuiltinValueKind::ICMP_SGT:
587+
TrueRel = ValueRelation::SLE;
588+
FalseRel = ValueRelation::SLT;
589+
Swap = true;
590+
break;
591+
case BuiltinValueKind::ICMP_ULE:
592+
TrueRel = ValueRelation::ULE;
593+
FalseRel = ValueRelation::ULT;
594+
break;
595+
case BuiltinValueKind::ICMP_ULT:
596+
TrueRel = ValueRelation::ULT;
597+
FalseRel = ValueRelation::ULE;
598+
break;
599+
case BuiltinValueKind::ICMP_UGT:
600+
TrueRel = ValueRelation::ULE;
601+
FalseRel = ValueRelation::ULT;
602+
Swap = true;
603+
break;
604+
case BuiltinValueKind::ICMP_UGE:
605+
TrueRel = ValueRelation::ULT;
606+
FalseRel = ValueRelation::ULE;
607+
Swap = true;
608+
break;
609+
}
610+
611+
SILValue Left = CMP->getOperand(0);
612+
SILValue Right = CMP->getOperand(1);
613+
614+
if (Swap)
615+
std::swap(Left, Right);
616+
617+
// Set the constraints for both side of the conditional branch, if
618+
// that the condition is dominating the dest block (see comment above).
619+
if (TrueBB) Constraints.push_back(Constraint(TrueBB, Left, Right, TrueRel));
620+
if (FalseBB) Constraints.push_back(Constraint(FalseBB, Right, Left, FalseRel));
621+
}
622+
518623
void registerCondFailFormula(CondFailInst *CFI) {
519624
// Extract the arithmetic operation from the condfail.
520-
auto *TEI = dyn_cast<TupleExtractInst>(CFI->getOperand());
521-
if (!TEI) return;
522-
auto *BI = dyn_cast<BuiltinInst>(TEI->getOperand());
523-
if (!BI) return;
625+
if (auto *TEI = dyn_cast<TupleExtractInst>(CFI->getOperand())) {
626+
auto *BI = dyn_cast<BuiltinInst>(TEI->getOperand());
627+
if (!BI)
628+
return;
524629

525-
// The relationship expressed in the builtin.
526-
ValueRelation Rel;
527-
switch (BI->getBuiltinInfo().ID) {
528-
default: return;
529-
case BuiltinValueKind::SAddOver:
530-
Rel = ValueRelation::SAdd;
531-
break;
532-
case BuiltinValueKind::UAddOver:
533-
Rel = ValueRelation::UAdd;
534-
break;
535-
case BuiltinValueKind::SSubOver:
536-
Rel = ValueRelation::SSub;
537-
break;
538-
case BuiltinValueKind::USubOver:
539-
Rel = ValueRelation::USub;
540-
break;
541-
case BuiltinValueKind::SMulOver:
542-
Rel = ValueRelation::SMul;
543-
break;
544-
case BuiltinValueKind::UMulOver:
545-
Rel = ValueRelation::UMul;
546-
break;
630+
// The relationship expressed in the builtin.
631+
Optional<ValueRelation> Rel = getArithOpRelation(BI);
632+
if (!Rel.hasValue())
633+
return;
634+
635+
// Construct and register the constraint.
636+
SILBasicBlock *Dom = CFI->getParent();
637+
SILValue Left = BI->getOperand(0);
638+
SILValue Right = BI->getOperand(1);
639+
Constraint F = Constraint(Dom, Left, Right, *Rel);
640+
Constraints.push_back(F);
547641
}
548642

549-
// Construct and register the constraint.
550-
SILBasicBlock *Dom = CFI->getParent();
551-
SILValue Left = BI->getOperand(0);
552-
SILValue Right = BI->getOperand(1);
553-
Constraint F = Constraint(Dom, Left, Right, Rel);
554-
Constraints.push_back(F);
643+
// Handle patterns like this:
644+
// %cmp_result = builtin "cmp_ult_Int64"
645+
// (%x : $Builtin.Int64, %y : $Builtin.Int64) : $Builtin.Int1
646+
// This cond_fail formula should be registered!
647+
// cond_fail %cmp_result : $Builtin.Int1
648+
// %check_underflow = integer_literal $Builtin.Int1, -1
649+
// At this point we know that x >= y
650+
// %usub_result = builtin "usub_with_overflow_Int64"
651+
// (%x : $Builtin.Int64, %y : $Builtin.Int64, %check_underflow : $Builtin.Int1):
652+
// $(Builtin.Int64, Builtin.Int1)
653+
// %usub_val = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 0
654+
// We can figure out that x - y will not underflow because of x >= y
655+
// %usub_underflow = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 1
656+
// cond_fail %usub_underflow : $Builtin.Int1
657+
if (auto *CMP = dyn_cast<BuiltinInst>(CFI->getOperand())) {
658+
SILBasicBlock *TrueBB = nullptr;
659+
SILBasicBlock *FalseBB = CMP->getParent();
660+
addComparisonRelation(CMP, TrueBB, FalseBB);
661+
}
555662
}
556663

664+
557665
void registerBranchFormula(CondBranchInst *BI) {
558666
// Extract the arithmetic operation from the Branch.
559667
auto *CMP = dyn_cast<BuiltinInst>(BI->getCondition());
@@ -583,68 +691,7 @@ class RedundantOverflowCheckRemovalPass : public SILFunctionTransform {
583691
if (!FalseBB->getSinglePredecessorBlock())
584692
FalseBB = nullptr;
585693

586-
// The relationship expressed in the builtin.
587-
ValueRelation Rel;
588-
bool Swap = false;
589-
590-
switch (CMP->getBuiltinInfo().ID) {
591-
default: return;
592-
case BuiltinValueKind::ICMP_NE: {
593-
SILValue Left = CMP->getOperand(0);
594-
SILValue Right = CMP->getOperand(1);
595-
if (FalseBB)
596-
Constraints.push_back(Constraint(FalseBB, Left, Right,
597-
ValueRelation::EQ));
598-
return;
599-
}
600-
case BuiltinValueKind::ICMP_EQ: {
601-
SILValue Left = CMP->getOperand(0);
602-
SILValue Right = CMP->getOperand(1);
603-
if (TrueBB)
604-
Constraints.push_back(Constraint(TrueBB, Left, Right,
605-
ValueRelation::EQ));
606-
return;
607-
}
608-
case BuiltinValueKind::ICMP_SLE:
609-
Rel = ValueRelation::SLE;
610-
break;
611-
case BuiltinValueKind::ICMP_SLT:
612-
Rel = ValueRelation::SLT;
613-
break;
614-
case BuiltinValueKind::ICMP_SGE:
615-
Rel = ValueRelation::SLT;
616-
Swap = true;
617-
break;
618-
case BuiltinValueKind::ICMP_SGT:
619-
Rel = ValueRelation::SLE;
620-
Swap = true;
621-
break;
622-
case BuiltinValueKind::ICMP_ULE:
623-
Rel = ValueRelation::ULE;
624-
break;
625-
case BuiltinValueKind::ICMP_ULT:
626-
Rel = ValueRelation::ULT;
627-
break;
628-
case BuiltinValueKind::ICMP_UGT:
629-
Rel = ValueRelation::ULE;
630-
Swap = true;
631-
break;
632-
case BuiltinValueKind::ICMP_UGE:
633-
Rel = ValueRelation::ULT;
634-
Swap = true;
635-
break;
636-
}
637-
638-
SILValue Left = CMP->getOperand(0);
639-
SILValue Right = CMP->getOperand(1);
640-
641-
if (Swap)
642-
std::swap(Left, Right);
643-
644-
// Set the constraints for both side of the conditional branch, if
645-
// that the condition is dominating the dest block (see comment above).
646-
if (TrueBB) Constraints.push_back(Constraint(TrueBB, Left, Right, Rel));
647-
if (FalseBB) Constraints.push_back(Constraint(FalseBB, Right, Left, Rel));
694+
addComparisonRelation(CMP, TrueBB, FalseBB );
648695
}
649696

650697
};

‎test/SILOptimizer/cropoverflow.sil

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,3 +743,36 @@ bb0(%0 : $Int8):
743743
%ret = tuple ()
744744
return %ret : $()
745745
}
746+
747+
// Check that the optimization can handle even patterns like this:
748+
// %cmp_result = builtin "cmp_ult_Int64"(%x : $Builtin.Int64, %y : $Builtin.Int64) : $Builtin.Int1
749+
// This cond_fail formula should be registered!
750+
// cond_fail %cmp_result : $Builtin.Int1
751+
// At this point we know that x >= y
752+
// %usub_result = builtin "usub_with_overflow_Int64"(%x : $Builtin.Int64, %y : $Builtin.Int64, %check_overfow : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1)
753+
// %usub_val = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 0
754+
// We can figure out that x - y will not underflow because of x >= y
755+
// %usub_underverflow = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 1
756+
// cond_fail %usub_underflow : $Builtin.Int1
757+
758+
// CHECK-LABEL: sil @cond_fail_of_cmp
759+
// CHECK: cond_fail
760+
// Second cond_fail should be removed, because it never fails.
761+
// CHECK-NOT: cond_fail
762+
// CHECK: // end sil function 'cond_fail_of_cmp'
763+
sil @cond_fail_of_cmp : $@convention(thin) (Builtin.Int64, Builtin.Int64) -> () {
764+
bb0(%x : $Builtin.Int64, %y: $Builtin.Int64):
765+
%cmp_result = builtin "cmp_ult_Int64"(%x : $Builtin.Int64, %y : $Builtin.Int64) : $Builtin.Int1
766+
// This cond_fail formula should be registered!
767+
cond_fail %cmp_result : $Builtin.Int1
768+
%check_overflow = integer_literal $Builtin.Int1, -1
769+
// At this point we know that x >= y
770+
%usub_result = builtin "usub_with_overflow_Int64"(%x : $Builtin.Int64, %y : $Builtin.Int64, %check_overflow : $Builtin.Int1) : $(Builtin.Int64, Builtin.Int1)
771+
%usub_val = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 0
772+
// We can figure out that x - y will not underflow because of x >= y
773+
%usub_underflow = tuple_extract %usub_result : $(Builtin.Int64, Builtin.Int1), 1
774+
cond_fail %usub_underflow : $Builtin.Int1
775+
776+
%ret = tuple ()
777+
return %ret : $()
778+
} // end sil function 'cond_fail_of_cmp'

0 commit comments

Comments
 (0)