[llvm] r219282 - [InstCombine] re-commit r218721 with fix for pr21199

Evgeniy Stepanov eugeni.stepanov at gmail.com
Wed Oct 8 09:03:43 PDT 2014


We also see miscompiles very likely originating from this commit
(disappeared at the latest revert in 219175, and back again with this
iteration). Sorry can't provide a test case.

On Wed, Oct 8, 2014 at 12:57 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



More information about the llvm-commits mailing list