[llvm] r219282 - [InstCombine] re-commit r218721 with fix for pr21199
Chandler Carruth
chandlerc at google.com
Wed Oct 8 01:57:20 PDT 2014
Looks like there are still problems here....
http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/7813
This is showing the bootstrapped clang failing its tests, most likely due
to a miscompile.
I'm pretty certain its this commit. If you look at the history of the bot,
these same tests failed in the same way when the patch first went in and
were fixed when Hans reverted it.
On Tue, Oct 7, 2014 at 11:42 PM, Gerolf Hoflehner <ghoflehner at apple.com>
wrote:
> Author: ghoflehner
> Date: Wed Oct 8 01:42:19 2014
> New Revision: 219282
>
> URL: http://llvm.org/viewvc/llvm-project?rev=219282&view=rev
> Log:
> [InstCombine] re-commit r218721 with fix for pr21199
>
> The icmp-select-icmp optimization targets select-icmp.eq
> only. This is now ensured by testing the branch predicate
> explictly. This commit also includes the test case for pr21199.
>
> Added:
> llvm/trunk/test/Transforms/InstCombine/pr21199.ll
> llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll
> Modified:
> llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
> llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> llvm/trunk/test/Transforms/InstCombine/pr12338.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombine.h?rev=219282&r1=219281&r2=219282&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombine.h (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombine.h Wed Oct 8
> 01:42:19 2014
> @@ -14,6 +14,7 @@
> #include "llvm/Analysis/AssumptionTracker.h"
> #include "llvm/Analysis/TargetFolder.h"
> #include "llvm/Analysis/ValueTracking.h"
> +#include "llvm/IR/Dominators.h"
> #include "llvm/IR/IRBuilder.h"
> #include "llvm/IR/InstVisitor.h"
> #include "llvm/IR/IntrinsicInst.h"
> @@ -98,7 +99,7 @@ class LLVM_LIBRARY_VISIBILITY InstCombin
> AssumptionTracker *AT;
> const DataLayout *DL;
> TargetLibraryInfo *TLI;
> - DominatorTree *DT; // not required
> + DominatorTree *DT;
> bool MadeIRChange;
> LibCallSimplifier *Simplifier;
> bool MinimizeSize;
> @@ -113,7 +114,8 @@ public:
> BuilderTy *Builder;
>
> static char ID; // Pass identification, replacement for typeid
> - InstCombiner() : FunctionPass(ID), DL(nullptr), Builder(nullptr) {
> + InstCombiner()
> + : FunctionPass(ID), DL(nullptr), DT(nullptr), Builder(nullptr) {
> MinimizeSize = false;
> initializeInstCombinerPass(*PassRegistry::getPassRegistry());
> }
> @@ -242,6 +244,11 @@ public:
>
> // visitInstruction - Specify what to return for unhandled
> instructions...
> Instruction *visitInstruction(Instruction &I) { return nullptr; }
> + bool dominatesAllUses(const Instruction *DI, const Instruction *UI,
> + const BasicBlock *DB) const;
> + bool replacedSelectWithOperand(SelectInst *SI, const ICmpInst *Icmp,
> + const ConstantInt *CI1,
> + const ConstantInt *CI2);
>
> private:
> bool ShouldChangeType(Type *From, Type *To) const;
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=219282&r1=219281&r2=219282&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Wed Oct
> 8 01:42:19 2014
> @@ -2429,6 +2429,125 @@ static bool swapMayExposeCSEOpportunitie
> return GlobalSwapBenefits > 0;
> }
>
> +/// \brief Check that one use is in the same block as the definition and
> all
> +/// other uses are in blocks dominated by a given block
> +///
> +/// \param DI Definition
> +/// \param UI Use
> +/// \param DB Block that must dominate all uses of \p DI outside
> +/// the parent block
> +/// \return true when \p UI is the only use of \p DI in the parent block
> +/// and all other uses of \p DI are in blocks dominated by \p DB.
> +///
> +bool InstCombiner::dominatesAllUses(const Instruction *DI,
> + const Instruction *UI,
> + const BasicBlock *DB) const {
> + assert(DI && UI && "Instruction not defined\n");
> + if (DI->getParent() != UI->getParent())
> + return false;
> + // DominatorTree available?
> + if (!DT)
> + return false;
> + for (const User *U : DI->users()) {
> + auto *Usr = cast<Instruction>(U);
> + if (Usr != UI && !DT->dominates(DB, Usr->getParent()))
> + return false;
> + }
> + return true;
> +}
> +
> +///
> +/// true when the instruction sequence within a block is select-cmp-br.
> +///
> +static bool isChainSelectCmpBranch(const SelectInst *SI) {
> + const BasicBlock *BB = SI->getParent();
> + if (!BB)
> + return false;
> + auto *BI = dyn_cast_or_null<BranchInst>(BB->getTerminator());
> + if (!BI || BI->getNumSuccessors() != 2)
> + return false;
> + auto *IC = dyn_cast<ICmpInst>(BI->getCondition());
> + if (!IC || (IC->getOperand(0) != SI && IC->getOperand(1) != SI))
> + return false;
> + return true;
> +}
> +
> +///
> +/// \brief True when a select result is replaced by one of its operands
> +/// in select-icmp sequence. This will eventually result in the
> elimination
> +/// of the select.
> +///
> +/// \param SI Select instruction
> +/// \param Icmp Compare instruction
> +/// \param CI1 'true' when first select operand is equal to RHSC of Icmp
> +/// \param CI2 'true' when second select operand is equal to RHSC of Icmp
> +///
> +/// Notes:
> +/// - The replacement is global and requires dominator information
> +/// - The caller is responsible for the actual replacement
> +///
> +/// Example:
> +///
> +/// entry:
> +/// %4 = select i1 %3, %C* %0, %C* null
> +/// %5 = icmp eq %C* %4, null
> +/// br i1 %5, label %9, label %7
> +/// ...
> +/// ; <label>:7 ; preds = %entry
> +/// %8 = getelementptr inbounds %C* %4, i64 0, i32 0
> +/// ...
> +///
> +/// can be transformed to
> +///
> +/// %5 = icmp eq %C* %0, null
> +/// %6 = select i1 %3, i1 %5, i1 true
> +/// br i1 %6, label %9, label %7
> +/// ...
> +/// ; <label>:7 ; preds = %entry
> +/// %8 = getelementptr inbounds %C* %0, i64 0, i32 0 // replace by %0!
> +///
> +/// Similar when the first operand of the select is a constant or/and
> +/// the compare is for not equal rather than equal.
> +///
> +/// FIXME: Currently the function considers equal compares only. It
> should be
> +/// possbile to extend it to not equal compares also.
> +///
> +bool InstCombiner::replacedSelectWithOperand(SelectInst *SI,
> + const ICmpInst *Icmp,
> + const ConstantInt *CI1,
> + const ConstantInt *CI2) {
> + if (isChainSelectCmpBranch(SI) &&
> + Icmp->getPredicate() == ICmpInst::ICMP_EQ) {
> + // Code sequence is select - icmp.eq - br
> + unsigned ReplaceWithOpd = 0;
> + if (CI1 && !CI1->isZero())
> + // The first constant operand of the select and the RHS of
> + // the compare match, so try to substitute
> + // the select results with its second operand
> + // Example:
> + // %4 = select i1 %3, %C* null, %C* %0
> + // %5 = icmp eq %C* %4, null
> + // ==> could replace select with second operand
> + ReplaceWithOpd = 2;
> + else if (CI2 && !CI2->isZero())
> + // Similar when the second operand of the select is a constant
> + // Example:
> + // %4 = select i1 %3, %C* %0, %C* null
> + // %5 = icmp eq %C* %4, null
> + // ==> could replace select with first operand
> + ReplaceWithOpd = 1;
> + if (ReplaceWithOpd) {
> + // Replace select with operand on else path for EQ compares.
> + BasicBlock *Succ =
> SI->getParent()->getTerminator()->getSuccessor(1);
> + if (InstCombiner::dominatesAllUses(SI, Icmp, Succ)) {
> + SI->replaceAllUsesWith(SI->getOperand(ReplaceWithOpd));
> + return true;
> + }
> + }
> + }
> + return false;
> +}
> +
> Instruction *InstCombiner::visitICmpInst(ICmpInst &I) {
> bool Changed = false;
> Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
> @@ -2885,8 +3004,21 @@ Instruction *InstCombiner::visitICmpInst
> // fold to a constant (in which case the icmp is replaced with a
> select
> // which will usually simplify) or this is the only user of the
> // select (in which case we are trading a select+icmp for a
> simpler
> - // select+icmp).
> - if ((Op1 && Op2) || (LHSI->hasOneUse() && (Op1 || Op2))) {
> + // select+icmp) or all uses of the select can be replaced based on
> + // dominance information ("Global cases").
> + bool Transform = false;
> + if (Op1 && Op2)
> + Transform = true;
> + else if (Op1 || Op2) {
> + if (LHSI->hasOneUse())
> + Transform = true;
> + else
> + // Global cases
> + Transform = replacedSelectWithOperand(
> + cast<SelectInst>(LHSI), &I,
> dyn_cast_or_null<ConstantInt>(Op1),
> + dyn_cast_or_null<ConstantInt>(Op2));
> + }
> + if (Transform) {
> if (!Op1)
> Op1 = Builder->CreateICmp(I.getPredicate(),
> LHSI->getOperand(1),
> RHSC, I.getName());
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp?rev=219282&r1=219281&r2=219282&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp Wed
> Oct 8 01:42:19 2014
> @@ -90,6 +90,7 @@ INITIALIZE_PASS_BEGIN(InstCombiner, "ins
> "Combine redundant instructions", false, false)
> INITIALIZE_PASS_DEPENDENCY(AssumptionTracker)
> INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfo)
> +INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
> INITIALIZE_PASS_END(InstCombiner, "instcombine",
> "Combine redundant instructions", false, false)
>
> @@ -97,6 +98,8 @@ void InstCombiner::getAnalysisUsage(Anal
> AU.setPreservesCFG();
> AU.addRequired<AssumptionTracker>();
> AU.addRequired<TargetLibraryInfo>();
> + AU.addRequired<DominatorTreeWrapperPass>();
> + AU.addPreserved<DominatorTreeWrapperPass>();
> }
>
>
> @@ -2933,12 +2936,9 @@ bool InstCombiner::runOnFunction(Functio
> AT = &getAnalysis<AssumptionTracker>();
> DataLayoutPass *DLP = getAnalysisIfAvailable<DataLayoutPass>();
> DL = DLP ? &DLP->getDataLayout() : nullptr;
> + DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
> TLI = &getAnalysis<TargetLibraryInfo>();
>
> - DominatorTreeWrapperPass *DTWP =
> - getAnalysisIfAvailable<DominatorTreeWrapperPass>();
> - DT = DTWP ? &DTWP->getDomTree() : nullptr;
> -
> // Minimizing size?
> MinimizeSize =
> F.getAttributes().hasAttribute(AttributeSet::FunctionIndex,
> Attribute::MinSize);
>
> Modified: llvm/trunk/test/Transforms/InstCombine/pr12338.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/pr12338.ll?rev=219282&r1=219281&r2=219282&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/pr12338.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/pr12338.ll Wed Oct 8 01:42:19
> 2014
> @@ -2,11 +2,11 @@
>
> define void @entry() nounwind {
> entry:
> +; CHECK: br label %for.cond
> br label %for.cond
>
> for.cond:
> %local = phi <1 x i32> [ <i32 0>, %entry ], [ %phi2, %cond.end47 ]
> -; CHECK: sub <1 x i32> <i32 92>, %local
> %phi3 = sub <1 x i32> zeroinitializer, %local
> br label %cond.end
>
>
> Added: llvm/trunk/test/Transforms/InstCombine/pr21199.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/pr21199.ll?rev=219282&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/pr21199.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/pr21199.ll Wed Oct 8 01:42:19
> 2014
> @@ -0,0 +1,25 @@
> +; do not replace a 'select' with 'or' in 'select - cmp - br' sequence
> +; RUN: opt -instcombine -S < %s | FileCheck %s
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> +declare void @f(i32)
> +
> +define void @test(i32 %len) {
> +entry:
> + %cmp = icmp ult i32 %len, 8
> + %cond = select i1 %cmp, i32 %len, i32 8
> + %cmp11 = icmp ult i32 0, %cond
> + br i1 %cmp11, label %for.body, label %for.end
> +
> +for.body: ; preds = %entry,
> %for.body
> + %i.02 = phi i32 [ %inc, %for.body ], [ 0, %entry ]
> + tail call void @f(i32 %cond)
> + %inc = add i32 %i.02, 1
> + %cmp1 = icmp ult i32 %inc, %cond
> + br i1 %cmp1, label %for.body, label %for.end
> +
> +for.end: ; preds = %for.body,
> %entry
> + ret void
> +; CHECK: select
> +}
>
> Added: llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll?rev=219282&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/select-cmp-br.ll Wed Oct 8
> 01:42:19 2014
> @@ -0,0 +1,127 @@
> +; Replace a 'select' with 'or' in 'select - cmp [eq|ne] - br' sequence
> +; RUN: opt -instcombine -S < %s | FileCheck %s
> +
> +%C = type <{ %struct.S }>
> +%struct.S = type { i64*, i32, i32 }
> +
> +declare void @bar(%struct.S *) #1
> +
> +define void @test1(%C*) {
> +entry:
> + %1 = getelementptr inbounds %C* %0, i64 0, i32 0, i32 0
> + %m = load i64** %1, align 8
> + %2 = getelementptr inbounds %C* %0, i64 1, i32 0, i32 0
> + %n = load i64** %2, align 8
> + %3 = getelementptr inbounds i64* %m, i64 9
> + %4 = bitcast i64* %3 to i64 (%C*)**
> + %5 = load i64 (%C*)** %4, align 8
> + %6 = icmp eq i64* %m, %n
> + %7 = select i1 %6, %C* %0, %C* null
> + %8 = icmp eq %C* %7, null
> + br i1 %8, label %12, label %10
> +
> +; <label>:9 ; preds = %10, %12
> + ret void
> +
> +; <label>:10 ; preds = %entry
> + %11 = getelementptr inbounds %C* %7, i64 0, i32 0
> + tail call void @bar(%struct.S* %11)
> + br label %9
> +
> +; <label>:12 ; preds = %entry
> + %13 = tail call i64 %5(%C* %0)
> + br label %9
> +; CHECK-LABEL: @test1(
> +; CHECK-NOT: select
> +; CHECK: or
> +}
> +
> +define void @test2(%C*) {
> +entry:
> + %1 = getelementptr inbounds %C* %0, i64 0, i32 0, i32 0
> + %m = load i64** %1, align 8
> + %2 = getelementptr inbounds %C* %0, i64 1, i32 0, i32 0
> + %n = load i64** %2, align 8
> + %3 = getelementptr inbounds i64* %m, i64 9
> + %4 = bitcast i64* %3 to i64 (%C*)**
> + %5 = load i64 (%C*)** %4, align 8
> + %6 = icmp eq i64* %m, %n
> + %7 = select i1 %6, %C* null, %C* %0
> + %8 = icmp eq %C* %7, null
> + br i1 %8, label %12, label %10
> +
> +; <label>:9 ; preds = %10, %12
> + ret void
> +
> +; <label>:10 ; preds = %entry
> + %11 = getelementptr inbounds %C* %7, i64 0, i32 0
> + tail call void @bar(%struct.S* %11)
> + br label %9
> +
> +; <label>:12 ; preds = %entry
> + %13 = tail call i64 %5(%C* %0)
> + br label %9
> +; CHECK-LABEL: @test2(
> +; CHECK-NOT: select
> +; CHECK: or
> +}
> +
> +define void @test3(%C*) {
> +entry:
> + %1 = getelementptr inbounds %C* %0, i64 0, i32 0, i32 0
> + %m = load i64** %1, align 8
> + %2 = getelementptr inbounds %C* %0, i64 1, i32 0, i32 0
> + %n = load i64** %2, align 8
> + %3 = getelementptr inbounds i64* %m, i64 9
> + %4 = bitcast i64* %3 to i64 (%C*)**
> + %5 = load i64 (%C*)** %4, align 8
> + %6 = icmp eq i64* %m, %n
> + %7 = select i1 %6, %C* %0, %C* null
> + %8 = icmp ne %C* %7, null
> + br i1 %8, label %10, label %12
> +
> +; <label>:9 ; preds = %10, %12
> + ret void
> +
> +; <label>:10 ; preds = %entry
> + %11 = getelementptr inbounds %C* %7, i64 0, i32 0
> + tail call void @bar(%struct.S* %11)
> + br label %9
> +
> +; <label>:12 ; preds = %entry
> + %13 = tail call i64 %5(%C* %0)
> + br label %9
> +; CHECK-LABEL: @test3(
> +; CHECK-NOT: select
> +; CHECK: or
> +}
> +
> +define void @test4(%C*) {
> +entry:
> + %1 = getelementptr inbounds %C* %0, i64 0, i32 0, i32 0
> + %m = load i64** %1, align 8
> + %2 = getelementptr inbounds %C* %0, i64 1, i32 0, i32 0
> + %n = load i64** %2, align 8
> + %3 = getelementptr inbounds i64* %m, i64 9
> + %4 = bitcast i64* %3 to i64 (%C*)**
> + %5 = load i64 (%C*)** %4, align 8
> + %6 = icmp eq i64* %m, %n
> + %7 = select i1 %6, %C* null, %C* %0
> + %8 = icmp ne %C* %7, null
> + br i1 %8, label %10, label %12
> +
> +; <label>:9 ; preds = %10, %12
> + ret void
> +
> +; <label>:10 ; preds = %entry
> + %11 = getelementptr inbounds %C* %7, i64 0, i32 0
> + tail call void @bar(%struct.S* %11)
> + br label %9
> +
> +; <label>:12 ; preds = %entry
> + %13 = tail call i64 %5(%C* %0)
> + br label %9
> +; CHECK-LABEL: @test4(
> +; CHECK-NOT: select
> +; CHECK: or
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141008/fb0accd8/attachment.html>
More information about the llvm-commits
mailing list