From 8ba674e43b5d907f2a0e3f651731232b7624c7df Mon Sep 17 00:00:00 2001 From: Xin Tong Date: Wed, 28 Feb 2018 12:08:00 +0000 Subject: [PATCH] [MergeICmp] Fix a bug in MergeICmp that can lead to a block being processed more than once. Summary: Fix a bug in MergeICmp that can lead to a BCECmp block being processed more than once and eventually lead to a broken LLVM module. The problem is that if the non-constant value is not produced by the last block, the producer will be processed once when the its parent block is processed and second time when the last block is processed. We end up having 2 same BCECmpBlock in the merge queue. And eventually lead to a broken LLVM module. Reviewers: courbet, davide Reviewed By: courbet Subscribers: llvm-commits Differential Revision: https://reviews.llvm.org/D43825 llvm-svn: 326318 --- llvm/lib/Transforms/Scalar/MergeICmps.cpp | 13 +++++ .../MergeICmps/X86/last-block-produce-no-value.ll | 57 ++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 llvm/test/Transforms/MergeICmps/X86/last-block-produce-no-value.ll diff --git a/llvm/lib/Transforms/Scalar/MergeICmps.cpp b/llvm/lib/Transforms/Scalar/MergeICmps.cpp index 745d29d..3e69512 100644 --- a/llvm/lib/Transforms/Scalar/MergeICmps.cpp +++ b/llvm/lib/Transforms/Scalar/MergeICmps.cpp @@ -571,6 +571,19 @@ bool processPhi(PHINode &Phi, const TargetLibraryInfo *const TLI) { DEBUG(dbgs() << "skip: several non-constant values\n"); return false; } + if (!isa(Phi.getIncomingValue(I)) || + cast(Phi.getIncomingValue(I))->getParent() != + Phi.getIncomingBlock(I)) { + // Non-constant incoming value is not from a cmp instruction or not + // produced by the last block. We could end up processing the value + // producing block more than once. + // + // This is an uncommon case, so we bail. + DEBUG( + dbgs() + << "skip: non-constant value not from cmp or not from last block.\n"); + return false; + } LastBlock = Phi.getIncomingBlock(I); } if (!LastBlock) { diff --git a/llvm/test/Transforms/MergeICmps/X86/last-block-produce-no-value.ll b/llvm/test/Transforms/MergeICmps/X86/last-block-produce-no-value.ll new file mode 100644 index 0000000..302b391 --- /dev/null +++ b/llvm/test/Transforms/MergeICmps/X86/last-block-produce-no-value.ll @@ -0,0 +1,57 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py +; RUN: opt < %s -mergeicmps -mtriple=x86_64-unknown-unknown -S | FileCheck %s + +%"struct.std::pair" = type { i32, i32, i32 } + +; Last block does not produce the non-constant value into the phi. +; We could handle this case, but an easier way would be to allow other transformations such as +; SimplifyCFG to remove %land.rhs.i.2 and turn the terminator of %land.rhs.i into an unconditional +; branch. + +define zeroext i1 @opeq1( +; CHECK-LABEL: @opeq1( +; CHECK-NEXT: entry: +; CHECK-NEXT: [[FIRST_I:%.*]] = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* [[A:%.*]], i64 0, i32 0 +; CHECK-NEXT: [[TMP0:%.*]] = load i32, i32* [[FIRST_I]], align 4 +; CHECK-NEXT: [[FIRST1_I:%.*]] = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* [[B:%.*]], i64 0, i32 0 +; CHECK-NEXT: [[TMP1:%.*]] = load i32, i32* [[FIRST1_I]], align 4 +; CHECK-NEXT: [[CMP_I:%.*]] = icmp eq i32 [[TMP0]], [[TMP1]] +; CHECK-NEXT: br i1 [[CMP_I]], label [[LAND_RHS_I:%.*]], label [[OPEQ1_EXIT:%.*]] +; CHECK: land.rhs.i: +; CHECK-NEXT: [[SECOND_I:%.*]] = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* [[A]], i64 0, i32 1 +; CHECK-NEXT: [[TMP2:%.*]] = load i32, i32* [[SECOND_I]], align 4 +; CHECK-NEXT: [[SECOND2_I:%.*]] = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* [[B]], i64 0, i32 1 +; CHECK-NEXT: [[TMP3:%.*]] = load i32, i32* [[SECOND2_I]], align 4 +; CHECK-NEXT: [[CMP3_I:%.*]] = icmp eq i32 [[TMP2]], [[TMP3]] +; CHECK-NEXT: br i1 [[CMP3_I]], label [[LAND_RHS_I:%.*]], label [[OPEQ1_EXIT:%.*]] +; CHECK: land.rhs.i.2: +; CHECK-NEXT: br label [[OPEQ1_EXIT]] +; CHECK: opeq1.exit: +; CHECK-NEXT: [[TMP4:%.*]] = phi i1 [ false, [[ENTRY:%.*]] ], [ [[CMP3_I]], [[LAND_RHS_I]] ] +; CHECK-NEXT: ret i1 [[TMP4]] +; + %"struct.std::pair"* nocapture readonly dereferenceable(12) %a, + %"struct.std::pair"* nocapture readonly dereferenceable(12) %b) local_unnamed_addr #0 { +entry: + %first.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %a, i64 0, i32 0 + %0 = load i32, i32* %first.i, align 4 + %first1.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %b, i64 0, i32 0 + %1 = load i32, i32* %first1.i, align 4 + %cmp.i = icmp eq i32 %0, %1 + br i1 %cmp.i, label %land.rhs.i, label %opeq1.exit + +land.rhs.i: + %second.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %a, i64 0, i32 1 + %2 = load i32, i32* %second.i, align 4 + %second2.i = getelementptr inbounds %"struct.std::pair", %"struct.std::pair"* %b, i64 0, i32 1 + %3 = load i32, i32* %second2.i, align 4 + %cmp3.i = icmp eq i32 %2, %3 + br i1 %cmp3.i, label %land.rhs.i.2, label %opeq1.exit + +land.rhs.i.2: + br label %opeq1.exit + +opeq1.exit: + %4 = phi i1 [ false, %entry ], [ false, %land.rhs.i], [ %cmp3.i, %land.rhs.i.2 ] + ret i1 %4 +} -- 2.7.4