[llvm] r286236 - [JumpThreading] Unfold selects that depend on the same condition
Pablo Barrio via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 22 05:48:31 PST 2016
Hello Haicheng,
I have been trying to reproduce the issue but it is working fine for me. I have run the LLVM test suite with the following command:
lnt runtest nt --no-timestamp --sandbox venv-llvm-tools --test-suite llvm-test-suite --cc /work/llvm-patched/bin/clang --cxx /work/llvm-patched/bin/clang++ --cflag -mcpu=cortex-a57 --cflag -O3 --cflag -flto --cflag -fuse-ld=gold --threads=8 --build-threads=8 --verbose
llvm-patched is a compiler built from top-of-tree with my two patches on top. I am compiling and running natively on a Cortex-A57.
Could you share the exact commands that you use for reproducing the issue? Do you run SciMark2 through lnt or do you invoke the benchmark directly? Are you cross-compiling? Any extra information you can give may be of help.
Many thanks,
Pablo
________________________________
From: Haicheng Wu <haicheng at codeaurora.org>
Sent: 15 November 2016 21:01
To: Pablo Barrio; mcrosier at codeaurora.org
Cc: llvm-commits at lists.llvm.org
Subject: RE: [llvm] r286236 - [JumpThreading] Unfold selects that depend on the same condition
Hi Pablo,
Thank you for doing this.
I also take another look at your patch. I think you may only need to unfold selects belonging to the same basic block. If they are in two different blocks, my understanding is that current jumpthreading implementation does not have the capability of optimizing the unfolded blocks without something like https://reviews.llvm.org/D22779.
Best,
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.
From: Pablo Barrio [mailto:Pablo.Barrio at arm.com]
Sent: Tuesday, November 15, 2016 10:15 AM
To: mcrosier at codeaurora.org
Cc: haicheng at codeaurora.org; llvm-commits at lists.llvm.org
Subject: Re: [llvm] r286236 - [JumpThreading] Unfold selects that depend on the same condition
Sure, I'll revert now.
________________________________
From: mcrosier at codeaurora.org<mailto:mcrosier at codeaurora.org> <mcrosier at codeaurora.org<mailto:mcrosier at codeaurora.org>>
Sent: 15 November 2016 14:18:20
To: Pablo Barrio
Cc: haicheng at codeaurora.org<mailto:haicheng at codeaurora.org>; llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r286236 - [JumpThreading] Unfold selects that depend on the same condition
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<mailto:haicheng at codeaurora.org> <haicheng at codeaurora.org<mailto:haicheng at codeaurora.org>>
> SENT: 15 November 2016 03:24:37
> TO: Pablo Barrio
> CC: llvm-commits at lists.llvm.org<mailto: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<mailto: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<mailto: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<mailto: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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161122/ccf857dd/attachment.html>
More information about the llvm-commits
mailing list