[llvm] r286236 - [JumpThreading] Unfold selects that depend on the same condition
via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 15 06:18:20 PST 2016
Hi Pablo,
Thanks for taking a look at this. If you don't think you'll have a fix
in the very near future would you mind reverting your commit while you
investigate?
Chad
On 2016-11-15 05:44, Pablo Barrio via llvm-commits wrote:
> Thank you Hai, I will have a look at it now.
>
> Pablo
>
> -------------------------
>
> FROM: haicheng at codeaurora.org <haicheng at codeaurora.org>
> SENT: 15 November 2016 03:24:37
> TO: Pablo Barrio
> CC: llvm-commits at lists.llvm.org
> SUBJECT: RE: [llvm] r286236 - [JumpThreading] Unfold selects that
> depend on the same condition
>
> Hi Pablo,
>
> It seems that your jumpthreading patch r286236 miscompiles
> llvm-test-suite/MultiSource/Benchmarks/SciMark2-C/scimark2 when using
> O3+LTO
> and running on AArch64.
>
> Here is the flag I used to compile " -O3 -flto -fuse-ld=gold
> -mcpu=cortex-a57"
> I used normal input size and got seg fault. Small input size seems
> okay.
>
> Please let me know if you need more information.
>
> Haicheng Wu
> Employee of Qualcomm Datacenter Technologies, Inc.
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
> Code
> Aurora Forum, a Linux Foundation Collaborative Project.
>
> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On
> Behalf Of
> Pablo Barrio via llvm-commits
> Sent: Tuesday, November 08, 2016 9:54 AM
> To: llvm-commits at lists.llvm.org
> Subject: [llvm] r286236 - [JumpThreading] Unfold selects that depend
> on
> the
> same condition
>
> Author: pabbar01
> Date: Tue Nov 8 08:53:30 2016
> New Revision: 286236
>
> URL: http://llvm.org/viewvc/llvm-project?rev=286236&view=rev
> Log:
> [JumpThreading] Unfold selects that depend on the same condition
>
> Summary:
> These are good candidates for jump threading. This enables later opts
> (such
> as InstCombine) to combine instructions from the selects with
> instructions
> out of the selects. SimplifyCFG will fold the select again if
> unfolding
> wasn't worth it.
>
> Patch by James Molloy and Pablo Barrio.
>
> Reviewers: rengolin, haicheng, sebpop
>
> Subscribers: jojo, jmolloy, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D26391
>
> Added:
>
> llvm/trunk/test/Transforms/JumpThreading/unfold-selects-same-cond.ll
> Modified:
> llvm/trunk/include/llvm/Transforms/Scalar/JumpThreading.h
> llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
>
> Modified: llvm/trunk/include/llvm/Transforms/Scalar/JumpThreading.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Scalar/JumpThreading.h?rev=286236&r1=286235&r2=286236&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Transforms/Scalar/JumpThreading.h
> (original)
> +++ llvm/trunk/include/llvm/Transforms/Scalar/JumpThreading.h Tue Nov
> 8
> +++ 08:53:30 2016
> @@ -129,6 +129,8 @@ private:
> BasicBlock *NewBB, BasicBlock
> *SuccBB);
> /// Check if the block has profile metadata for its outgoing
> edges.
> bool doesBlockHaveProfileData(BasicBlock *BB);
> + SelectInst *getSelectFedByPhi(PHINode *PN); void
> + expandSelect(SelectInst *SI);
> };
>
> } // end namespace llvm
>
> Modified: llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp?rev=286236&r1=286235&r2=286236&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/JumpThreading.cpp Tue Nov 8
> +++ 08:53:30 2016
> @@ -1963,61 +1963,100 @@ bool JumpThreadingPass::TryToUnfoldSelec
> return false;
> }
>
> -/// TryToUnfoldSelectInCurrBB - Look for PHI/Select in the same BB of
>
> the
> form
> +/// GetSelectFedByPhi - Look for PHI/Select in the same BB of the
> form
> /// bb:
> /// %p = phi [false, %bb1], [true, %bb2], [false, %bb3], [true,
> %bb4],
> ...
> /// %s = select p, trueval, falseval
> ///
> -/// And expand the select into a branch structure. This later enables
> +/// And return the select. Unfolding it into a branch structure later
> +enables
> /// jump-threading over bb in this pass.
> ///
> -/// Using the similar approach of SimplifyCFG::FoldCondBranchOnPHI(),
> unfold -/// select if the associated PHI has at least one constant.
> If
> the
> unfolded -/// select is not jump-threaded, it will be folded again in
> the
> later -/// optimizations.
> +/// Using the similar approach of SimplifyCFG::FoldCondBranchOnPHI(),
> +return /// select if the associated PHI has at least one constant.
> +SelectInst *JumpThreadingPass::getSelectFedByPhi(PHINode *PN) {
> +
> + unsigned NumPHIValues = PN->getNumIncomingValues(); if
> (NumPHIValues
> + == 0 || !PN->hasOneUse())
> + return nullptr;
> +
> + SelectInst *SI = dyn_cast<SelectInst>(PN->user_back());
> + BasicBlock *BB = PN->getParent();
> + if (!SI || SI->getParent() != BB)
> + return nullptr;
> +
> + Value *Cond = SI->getCondition();
> + if (!Cond || Cond != PN || !Cond->getType()->isIntegerTy(1))
> + return nullptr;
> +
> + for (unsigned i = 0; i != NumPHIValues; ++i) {
> + if (PN->getIncomingBlock(i) == BB)
> + return nullptr;
> + if (isa<ConstantInt>(PN->getIncomingValue(i)))
> + return SI;
> + }
> +
> + return nullptr;
> +}
> +
> +/// ExpandSelect - Expand a select into an if-then-else construct.
> +void JumpThreadingPass::expandSelect(SelectInst *SI) {
> +
> + BasicBlock *BB = SI->getParent();
> + TerminatorInst *Term =
> + SplitBlockAndInsertIfThen(SI->getCondition(), SI, false);
> + PHINode *NewPN = PHINode::Create(SI->getType(), 2, "", SI);
> + NewPN->addIncoming(SI->getTrueValue(), Term->getParent());
> + NewPN->addIncoming(SI->getFalseValue(), BB);
> + SI->replaceAllUsesWith(NewPN);
> + SI->eraseFromParent();
> +}
> +
> +/// TryToUnfoldSelectInCurrBB - Unfold selects that could be
> +jump-threaded were /// they if-then-elses. If the unfolded selects
> are
> +not jump-threaded, it will /// be folded again in the later
> optimizations.
> bool JumpThreadingPass::TryToUnfoldSelectInCurrBB(BasicBlock *BB) {
> +
> // If threading this would thread across a loop header, don't
> thread
> the
> edge.
> // See the comments above FindLoopHeaders for justifications and
> caveats.
> if (LoopHeaders.count(BB))
> return false;
>
> - // Look for a Phi/Select pair in the same basic block. The Phi
> feeds
> the
> - // condition of the Select and at least one of the incoming values
> is
> a
> - // constant.
> - for (BasicBlock::iterator BI = BB->begin();
> - PHINode *PN = dyn_cast<PHINode>(BI); ++BI) {
> - unsigned NumPHIValues = PN->getNumIncomingValues();
> - if (NumPHIValues == 0 || !PN->hasOneUse())
> - continue;
> + bool Changed = false;
> + for (auto &I : *BB) {
>
> - SelectInst *SI = dyn_cast<SelectInst>(PN->user_back());
> - if (!SI || SI->getParent() != BB)
> + // Look for a Phi/Select pair in the same basic block. The Phi
> feeds
> the
> + // condition of the Select and at least one of the incoming
> values
> is a
> + // constant.
> + PHINode *PN;
> + SelectInst *SI;
> + if ((PN = dyn_cast<PHINode>(&I)) && (SI = getSelectFedByPhi(PN)))
> {
> + expandSelect(SI);
> + Changed = true;
> continue;
> + }
>
> - Value *Cond = SI->getCondition();
> - if (!Cond || Cond != PN || !Cond->getType()->isIntegerTy(1))
> - continue;
> + if (I.getType()->isIntegerTy(1)) {
>
> - bool HasConst = false;
> - for (unsigned i = 0; i != NumPHIValues; ++i) {
> - if (PN->getIncomingBlock(i) == BB)
> - return false;
> - if (isa<ConstantInt>(PN->getIncomingValue(i)))
> - HasConst = true;
> - }
> + SmallVector<SelectInst *, 4> Selects;
>
> - if (HasConst) {
> - // Expand the select.
> - TerminatorInst *Term =
> - SplitBlockAndInsertIfThen(SI->getCondition(), SI, false);
> - PHINode *NewPN = PHINode::Create(SI->getType(), 2, "", SI);
> - NewPN->addIncoming(SI->getTrueValue(), Term->getParent());
> - NewPN->addIncoming(SI->getFalseValue(), BB);
> - SI->replaceAllUsesWith(NewPN);
> - SI->eraseFromParent();
> - return true;
> + // Look for scalar booleans used in selects as conditions. If
> there
> are
> + // several selects that use the same boolean, they are
> candidates
> for
> jump
> + // threading and therefore we should unfold them.
> + for (Value *U : I.users())
> + if (auto *SI = dyn_cast<SelectInst>(U))
> + Selects.push_back(SI);
> + if (Selects.size() <= 1)
> + continue;
> +
> + // Remove duplicates
> + std::sort(Selects.begin(), Selects.end());
> + auto NewEnd = std::unique(Selects.begin(), Selects.end());
> +
> + Changed = true;
> + for (auto SI = Selects.begin(); SI != NewEnd; ++SI)
> + expandSelect(*SI);
> }
> }
> -
> - return false;
> +
> + return Changed;
> }
>
> Added:
> llvm/trunk/test/Transforms/JumpThreading/unfold-selects-same-cond.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/JumpThreading/unfold-selects-same-cond.ll?rev=286236&view=auto
> ==============================================================================
> ---
> llvm/trunk/test/Transforms/JumpThreading/unfold-selects-same-cond.ll
> (added)
> +++
> llvm/trunk/test/Transforms/JumpThreading/unfold-selects-same-cond.ll
> +++ Tue Nov 8 08:53:30 2016
> @@ -0,0 +1,45 @@
> +; RUN: opt < %s -jump-threading -instcombine -simplifycfg -S |
> +FileCheck %s
> +
> +; The three selects are jump-threaded so that instcombine can
> optimize,
> +and ; simplifycfg should turn the result into a single select.
> +define i32 @f(i32 %a, i32 %b) {
> +; CHECK: select
> +; CHECK-NOT: select
> +entry:
> + %0 = and i32 %a, 1
> + %1 = and i32 %b, 1
> + %xor = xor i32 %1, %a
> + %shr32 = lshr i32 %a, 1
> + %cmp10 = icmp eq i32 %xor, 1
> + %2 = xor i32 %b, 12345
> + %b.addr.1 = select i1 %cmp10, i32 %2, i32 %b
> + %shr1633 = lshr i32 %b.addr.1, 1
> + %3 = or i32 %shr1633, 54321
> + %b.addr.2 = select i1 %cmp10, i32 %3, i32 %shr1633
> + %shr1634 = lshr i32 %b.addr.2, 2
> + %4 = or i32 %shr1634, 54320
> + %b.addr.3 = select i1 %cmp10, i32 %4, i32 %shr1634
> + ret i32 %b.addr.3
> +}
> +
> +; Case where the condition is not only used as condition but also as
> +the ; true or false value in at least one of the selects.
> +define i1 @g(i32 %a, i32 %b) {
> +; CHECK: select
> +; CHECK-NOT: select
> +entry:
> + %0 = and i32 %a, 1
> + %1 = and i32 %b, 1
> + %xor = xor i32 %1, %a
> + %shr32 = lshr i32 %a, 1
> + %cmp10 = icmp eq i32 %xor, 1
> + %2 = xor i32 %b, 12345
> + %b.addr.1 = select i1 %cmp10, i32 %2, i32 %b
> + %shr1633 = lshr i32 %b.addr.1, 1
> + %3 = or i32 %shr1633, 54321
> + %b.addr.2 = select i1 %cmp10, i32 %3, i32 %shr1633
> + %shr1634 = lshr i32 %b.addr.2, 2
> + %4 = icmp eq i32 %shr1634, 54320
> + %b.addr.3 = select i1 %cmp10, i1 %4, i1 %cmp10
> + ret i1 %b.addr.3
> +}
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose
> the contents to any other person, use it for any purpose, or store or
> copy the information in any medium. Thank you.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list