This is an archive of the discontinued LLVM Phabricator instance.

Differential D44083

[MergICmps] Make sure that the comparison only has one use.
ClosedPublic

Authored by courbet on Mar 5 2018, 2:05 AM.

Details

Reviewers
trentxintong
spatel
Commits
rG9f0b3170bcea: [MergeICmps] Make sure that the comparison only has one use.
rL327372: [MergeICmps] Make sure that the comparison only has one use.
Summary

Fixes PR36557.

Diff Detail

Event Timeline

courbet created this revision.Mar 5 2018, 2:05 AM
courbet added a comment.Mar 12 2018, 1:48 AM

Ping

spatel added inline comments.Mar 12 2018, 6:43 AM
lib/Transforms/Scalar/MergeICmps.cpp
186–188

The change looks safe and fixes the bug. But I haven't looked at the whole pass, so it's not clear to me why we should have the restriction. Can you add something to the comment to explain?

spatel added inline comments.Mar 12 2018, 6:44 AM
test/Transforms/MergeICmps/X86/pr36557.ll
101–118

Should be able to remove some of this and still show the bug?

courbet updated this revision to Diff 138016.Mar 12 2018, 8:27 AM
courbet marked an inline comment as done.
  • Remove superfluous annotations in test.
  • add more comments
courbet marked an inline comment as done.Mar 12 2018, 8:28 AM
courbet added inline comments.
test/Transforms/MergeICmps/X86/pr36557.ll
101–118

All, actually :)

Harbormaster completed remote builds in B15966: Diff 138016.Mar 12 2018, 8:29 AM
spatel accepted this revision.Mar 12 2018, 8:40 AM

LGTM.

lib/Transforms/Scalar/MergeICmps.cpp
190

at -> as

This revision is now accepted and ready to land.Mar 12 2018, 8:40 AM
Closed by commit rL327372: [MergeICmps] Make sure that the comparison only has one use. (authored by courbet). · Explain WhyMar 13 2018, 12:09 AM
This revision was automatically updated to reflect the committed changes.
courbet marked an inline comment as done.
courbet added a comment.Mar 13 2018, 4:02 AM

Thanks.

Revision Contents

PathSize
lib/
Transforms/
Scalar/
7 lines
test/
Transforms/
MergeICmps/
X86/
119 lines

Diff 136960

lib/Transforms/Scalar/MergeICmps.cpp

Show First 20 Lines • Show All 177 Lines • ▼ Show 20 Linesbool BCECmpBlock::doesOtherWork() const {
} }
return false; return false;
} }
// Visit the given comparison. If this is a comparison between two valid // Visit the given comparison. If this is a comparison between two valid
// BCE atoms, returns the comparison. // BCE atoms, returns the comparison.
BCECmpBlock visitICmp(const ICmpInst *const CmpI, BCECmpBlock visitICmp(const ICmpInst *const CmpI,
const ICmpInst::Predicate ExpectedPredicate) { const ICmpInst::Predicate ExpectedPredicate) {
// The comparison can only be used once:
// - For intermediate blocks, as a branch condition.
// - For the final block, as an incoming value for the Phi.
spatelUnsubmitted
Done
ReplyInline Actions

The change looks safe and fixes the bug. But I haven't looked at the whole pass, so it's not clear to me why we should have the restriction. Can you add something to the comment to explain?

spatel: The change looks safe and fixes the bug. But I haven't looked at the whole pass, so it's not…
if (!CmpI->hasOneUse()) {
DEBUG(dbgs() << "cmp has several uses\n");
spatelUnsubmitted
Not Done
ReplyInline Actions

at -> as

spatel: at -> as
return {};
}
if (CmpI->getPredicate() == ExpectedPredicate) { if (CmpI->getPredicate() == ExpectedPredicate) {
DEBUG(dbgs() << "cmp " DEBUG(dbgs() << "cmp "
<< (ExpectedPredicate == ICmpInst::ICMP_EQ ? "eq" : "ne") << (ExpectedPredicate == ICmpInst::ICMP_EQ ? "eq" : "ne")
<< "\n"); << "\n");
auto Lhs = visitICmpLoadOperand(CmpI->getOperand(0)); auto Lhs = visitICmpLoadOperand(CmpI->getOperand(0));
if (!Lhs.Base()) return {}; if (!Lhs.Base()) return {};
auto Rhs = visitICmpLoadOperand(CmpI->getOperand(1)); auto Rhs = visitICmpLoadOperand(CmpI->getOperand(1));
if (!Rhs.Base()) return {}; if (!Rhs.Base()) return {};
▲ Show 20 Lines • Show All 477 Lines • Show Last 20 Lines

test/Transforms/MergeICmps/X86/pr36557.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -mergeicmps -mtriple=x86_64-unknown-unknown -S | FileCheck %s --check-prefix=X86
source_filename = "qabstractitemmodeltester.cpp"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64--linux-gnu"
%class.B = type { i32, i32 }
%class.D = type { i32 }
%class.C = type { i8 }
%class.QMessageLogger = type { i8 }
$_ZN1D7compareI1BS1_EEbRKT_RKT0_PKcS9_S9_i = comdat any
@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
; Function Attrs: uwtable
define linkonce_odr dso_local zeroext i1 @_ZN1D7compareI1BS1_EEbRKT_RKT0_PKcS9_S9_i(%class.D* %this, %class.B* dereferenceable(8) %p1, %class.B* dereferenceable(8) %p2, i8*, i8*, i8*, i32) local_unnamed_addr #0 comdat align 2 {
; X86-LABEL: @_ZN1D7compareI1BS1_EEbRKT_RKT0_PKcS9_S9_i(
; X86-NEXT: entry:
; X86-NEXT: [[REF_TMP:%.*]] = alloca [[CLASS_C:%.*]], align 1
; X86-NEXT: [[C_I:%.*]] = getelementptr inbounds [[CLASS_B:%.*]], %class.B* [[P2:%.*]], i64 0, i32 0
; X86-NEXT: [[TMP4:%.*]] = load i32, i32* [[C_I]], align 4, !tbaa !2
; X86-NEXT: [[C2_I:%.*]] = getelementptr inbounds [[CLASS_B]], %class.B* [[P1:%.*]], i64 0, i32 0
; X86-NEXT: [[TMP5:%.*]] = load i32, i32* [[C2_I]], align 4, !tbaa !2
; X86-NEXT: [[CMP_ENTRY:%.*]] = icmp eq i32 [[TMP4]], [[TMP5]]
; X86-NEXT: br i1 [[CMP_ENTRY]], label [[BB1:%.*]], label [[BB2:%.*]]
; X86: bb1:
; X86-NEXT: [[M_I:%.*]] = getelementptr inbounds [[CLASS_B]], %class.B* [[P2]], i64 0, i32 1
; X86-NEXT: [[TMP6:%.*]] = load i32, i32* [[M_I]], align 4, !tbaa !7
; X86-NEXT: [[M3_I:%.*]] = getelementptr inbounds [[CLASS_B]], %class.B* [[P1]], i64 0, i32 1
; X86-NEXT: [[TMP7:%.*]] = load i32, i32* [[M3_I]], align 4, !tbaa !7
; X86-NEXT: [[CMP1:%.*]] = icmp eq i32 [[TMP6]], [[TMP7]]
; X86-NEXT: br label [[BB2]]
; X86: bb2:
; X86-NEXT: [[TMP8:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[CMP1]], [[BB1]] ]
; X86-NEXT: [[FAILUREREPORTINGMODE:%.*]] = getelementptr inbounds [[CLASS_D:%.*]], %class.D* [[THIS:%.*]], i64 0, i32 0
; X86-NEXT: [[TMP9:%.*]] = load i32, i32* [[FAILUREREPORTINGMODE]], align 4, !tbaa !8
; X86-NEXT: [[COND:%.*]] = icmp eq i32 [[TMP9]], 0
; X86-NEXT: br i1 [[COND]], label [[BB3:%.*]], label [[SW_EPILOG:%.*]]
; X86: bb3:
; X86-NEXT: br i1 [[CMP_ENTRY]], label [[BB4:%.*]], label [[BB5:%.*]]
; X86: bb4:
; X86-NEXT: [[M_I_I:%.*]] = getelementptr inbounds [[CLASS_B]], %class.B* [[P2]], i64 0, i32 1
; X86-NEXT: [[TMP10:%.*]] = load i32, i32* [[M_I_I]], align 4, !tbaa !7
; X86-NEXT: [[M3_I_I:%.*]] = getelementptr inbounds [[CLASS_B]], %class.B* [[P1]], i64 0, i32 1
; X86-NEXT: [[TMP11:%.*]] = load i32, i32* [[M3_I_I]], align 4, !tbaa !7
; X86-NEXT: [[CMP4:%.*]] = icmp eq i32 [[TMP10]], [[TMP11]]
; X86-NEXT: br label [[BB5]]
; X86: bb5:
; X86-NEXT: [[TMP12:%.*]] = phi i1 [ false, [[BB3]] ], [ [[CMP4]], [[BB4]] ]
; X86-NEXT: [[TMP13:%.*]] = getelementptr inbounds [[CLASS_C]], %class.C* [[REF_TMP]], i64 0, i32 0
; X86-NEXT: br label [[SW_EPILOG]]
; X86: sw.epilog:
; X86-NEXT: ret i1 [[TMP8]]
;
entry:
%ref.tmp = alloca %class.C, align 1
%c.i = getelementptr inbounds %class.B, %class.B* %p2, i64 0, i32 0
%4 = load i32, i32* %c.i, align 4, !tbaa !2
%c2.i = getelementptr inbounds %class.B, %class.B* %p1, i64 0, i32 0
%5 = load i32, i32* %c2.i, align 4, !tbaa !2
%cmp_entry = icmp eq i32 %4, %5
br i1 %cmp_entry, label %bb1, label %bb2
bb1: ; preds = %entry
%m.i = getelementptr inbounds %class.B, %class.B* %p2, i64 0, i32 1
%6 = load i32, i32* %m.i, align 4, !tbaa !7
%m3.i = getelementptr inbounds %class.B, %class.B* %p1, i64 0, i32 1
%7 = load i32, i32* %m3.i, align 4, !tbaa !7
%cmp1 = icmp eq i32 %6, %7
br label %bb2
bb2: ; preds = %entry, %bb1
%8 = phi i1 [ false, %entry ], [ %cmp1, %bb1 ]
%failureReportingMode = getelementptr inbounds %class.D, %class.D* %this, i64 0, i32 0
%9 = load i32, i32* %failureReportingMode, align 4, !tbaa !8
%cond = icmp eq i32 %9, 0
br i1 %cond, label %bb3, label %sw.epilog
bb3: ; preds = %bb2
br i1 %cmp_entry, label %bb4, label %bb5
bb4: ; preds = %bb3
%m.i.i = getelementptr inbounds %class.B, %class.B* %p2, i64 0, i32 1
%10 = load i32, i32* %m.i.i, align 4, !tbaa !7
%m3.i.i = getelementptr inbounds %class.B, %class.B* %p1, i64 0, i32 1
%11 = load i32, i32* %m3.i.i, align 4, !tbaa !7
%cmp4 = icmp eq i32 %10, %11
br label %bb5
bb5: ; preds = %bb3, %bb4
%12 = phi i1 [ false, %bb3 ], [ %cmp4, %bb4 ]
%13 = getelementptr inbounds %class.C, %class.C* %ref.tmp, i64 0, i32 0
br label %sw.epilog
sw.epilog: ; preds = %bb2
ret i1 %8
}
attributes #0 = { uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
attributes #2 = { argmemonly nounwind }
attributes #3 = { nounwind }
!llvm.module.flags = !{!0}
!llvm.ident = !{!1}
!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{!"clang version 7.0.0 (trunk 326236) (llvm/trunk 326244)"}
!2 = !{!3, !4, i64 0}
!3 = !{!"_ZTS1B", !4, i64 0, !4, i64 4}
!4 = !{!"int", !5, i64 0}
!5 = !{!"omnipotent char", !6, i64 0}
!6 = !{!"Simple C++ TBAA"}
!7 = !{!3, !4, i64 4}
!8 = !{!9, !10, i64 0}
!9 = !{!"_ZTS1D", !10, i64 0}
spatelUnsubmitted
Done
ReplyInline Actions

Should be able to remove some of this and still show the bug?

spatel: Should be able to remove some of this and still show the bug?
courbetAuthorUnsubmitted
Not Done
ReplyInline Actions

All, actually :)

courbet: All, actually :)
!10 = !{!"_ZTSN1A20FailureReportingModeE", !5, i64 0}