<div dir="ltr">Thanks, Benjamin. Is there a recommended procedure to reduce a test case from that failure?<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Oct 16, 2015 at 5:02 PM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sanjay,<br>
<br>
looks like r250527 breaks clang selfhosting, see PR25222. I've<br>
reverted the commit for now in r250570.<br>
<br>
- Ben<br>
<div class="HOEnZb"><div class="h5"><br>
On Fri, Oct 16, 2015 at 6:54 PM, Sanjay Patel via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: spatel<br>
> Date: Fri Oct 16 11:54:30 2015<br>
> New Revision: 250527<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=250527&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=250527&view=rev</a><br>
> Log:<br>
> This is a follow-up to the discussion in D12882.<br>
><br>
> Ideally, we would like SimplifyCFG to be able to form select instructions even when the operands<br>
> are expensive (as defined by the TTI cost model) because that may expose further optimizations.<br>
> However, we would then like a later pass like CodeGenPrepare to undo that transformation if the<br>
> target would likely benefit from not speculatively executing an expensive op (this patch).<br>
><br>
> Once we have this safety mechanism in place, we can adjust SimplifyCFG to restore its<br>
> select-formation behavior that changed with r248439.<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D13297" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13297</a><br>
><br>
><br>
> Added:<br>
> llvm/trunk/test/Transforms/CodeGenPrepare/select.ll<br>
> Modified:<br>
> llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp<br>
><br>
> Modified: llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp?rev=250527&r1=250526&r2=250527&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp?rev=250527&r1=250526&r2=250527&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp (original)<br>
> +++ llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp Fri Oct 16 11:54:30 2015<br>
> @@ -3846,8 +3846,20 @@ bool CodeGenPrepare::optimizeExtUses(Ins<br>
> return MadeChange;<br>
> }<br>
><br>
> +/// Check if V (an operand of a select instruction) is an expensive instruction<br>
> +/// that is only used once.<br>
> +static bool sinkSelectOperand(const TargetTransformInfo *TTI, Value *V) {<br>
> + auto *I = dyn_cast<Instruction>(V);<br>
> + if (I && I->hasOneUse() &&<br>
> + TTI->getUserCost(I) == TargetTransformInfo::TCC_Expensive)<br>
> + return true;<br>
> +<br>
> + return false;<br>
> +}<br>
> +<br>
> /// Returns true if a SelectInst should be turned into an explicit branch.<br>
> -static bool isFormingBranchFromSelectProfitable(SelectInst *SI) {<br>
> +static bool isFormingBranchFromSelectProfitable(const TargetTransformInfo *TTI,<br>
> + SelectInst *SI) {<br>
> // FIXME: This should use the same heuristics as IfConversion to determine<br>
> // whether a select is better represented as a branch. This requires that<br>
> // branch probability metadata is preserved for the select, which is not the<br>
> @@ -3868,8 +3880,17 @@ static bool isFormingBranchFromSelectPro<br>
> // on a load from memory. But if the load is used more than once, do not<br>
> // change the select to a branch because the load is probably needed<br>
> // regardless of whether the branch is taken or not.<br>
> - return ((isa<LoadInst>(CmpOp0) && CmpOp0->hasOneUse()) ||<br>
> - (isa<LoadInst>(CmpOp1) && CmpOp1->hasOneUse()));<br>
> + if ((isa<LoadInst>(CmpOp0) && CmpOp0->hasOneUse()) ||<br>
> + (isa<LoadInst>(CmpOp1) && CmpOp1->hasOneUse()))<br>
> + return true;<br>
> +<br>
> + // If either operand of the select is expensive and only needed on one side<br>
> + // of the select, we should form a branch.<br>
> + if (sinkSelectOperand(TTI, SI->getTrueValue()) ||<br>
> + sinkSelectOperand(TTI, SI->getFalseValue()))<br>
> + return true;<br>
> +<br>
> + return false;<br>
> }<br>
><br>
><br>
> @@ -3895,34 +3916,97 @@ bool CodeGenPrepare::optimizeSelectInst(<br>
> // We have efficient codegen support for the select instruction.<br>
> // Check if it is profitable to keep this 'select'.<br>
> if (!TLI->isPredictableSelectExpensive() ||<br>
> - !isFormingBranchFromSelectProfitable(SI))<br>
> + !isFormingBranchFromSelectProfitable(TTI, SI))<br>
> return false;<br>
> }<br>
><br>
> ModifiedDT = true;<br>
><br>
> + // Transform a sequence like this:<br>
> + // start:<br>
> + // %cmp = cmp uge i32 %a, %b<br>
> + // %sel = select i1 %cmp, i32 %c, i32 %d<br>
> + //<br>
> + // Into:<br>
> + // start:<br>
> + // %cmp = cmp uge i32 %a, %b<br>
> + // br i1 %cmp, label %select.true, label %select.false<br>
> + // select.true:<br>
> + // br label %select.end<br>
> + // select.false:<br>
> + // br label %select.end<br>
> + // select.end:<br>
> + // %sel = phi i32 [ %c, %select.true ], [ %d, %select.false ]<br>
> + //<br>
> + // In addition, we may sink instructions that produce %c or %d from<br>
> + // the entry block into the destination(s) of the new branch.<br>
> + // If the true or false blocks do not contain a sunken instruction, that<br>
> + // block and its branch may be optimized away. In that case, one side of the<br>
> + // first branch will point directly to select.end, and the corresponding PHI<br>
> + // predecessor block will be the start block.<br>
> +<br>
> // First, we split the block containing the select into 2 blocks.<br>
> BasicBlock *StartBlock = SI->getParent();<br>
> BasicBlock::iterator SplitPt = ++(BasicBlock::iterator(SI));<br>
> - BasicBlock *NextBlock = StartBlock->splitBasicBlock(SplitPt, "select.end");<br>
> -<br>
> - // Create a new block serving as the landing pad for the branch.<br>
> - BasicBlock *SmallBlock = BasicBlock::Create(SI->getContext(), "select.mid",<br>
> - NextBlock->getParent(), NextBlock);<br>
> + BasicBlock *EndBlock = StartBlock->splitBasicBlock(SplitPt, "select.end");<br>
><br>
> - // Move the unconditional branch from the block with the select in it into our<br>
> - // landing pad block.<br>
> + // Delete the unconditional branch that was just created by the split.<br>
> StartBlock->getTerminator()->eraseFromParent();<br>
> - BranchInst::Create(NextBlock, SmallBlock);<br>
> +<br>
> + // These are the new basic blocks for the conditional branch.<br>
> + // At least one will become an actual new basic block.<br>
> + BasicBlock *TrueBlock = nullptr;<br>
> + BasicBlock *FalseBlock = nullptr;<br>
> +<br>
> + // Sink expensive instructions into the conditional blocks to avoid executing<br>
> + // them speculatively.<br>
> + if (sinkSelectOperand(TTI, SI->getTrueValue())) {<br>
> + TrueBlock = BasicBlock::Create(SI->getContext(), "select.true.sink",<br>
> + EndBlock->getParent(), EndBlock);<br>
> + auto *TrueBranch = BranchInst::Create(EndBlock, TrueBlock);<br>
> + auto *TrueInst = cast<Instruction>(SI->getTrueValue());<br>
> + TrueInst->moveBefore(TrueBranch);<br>
> + }<br>
> + if (sinkSelectOperand(TTI, SI->getFalseValue())) {<br>
> + FalseBlock = BasicBlock::Create(SI->getContext(), "select.false.sink",<br>
> + EndBlock->getParent(), EndBlock);<br>
> + auto *FalseBranch = BranchInst::Create(EndBlock, FalseBlock);<br>
> + auto *FalseInst = cast<Instruction>(SI->getFalseValue());<br>
> + FalseInst->moveBefore(FalseBranch);<br>
> + }<br>
> +<br>
> + // If there was nothing to sink, then arbitrarily choose the 'false' side<br>
> + // for a new input value to the PHI.<br>
> + if (TrueBlock == FalseBlock) {<br>
> + assert(TrueBlock == nullptr &&<br>
> + "Unexpected basic block transform while optimizing select");<br>
> +<br>
> + FalseBlock = BasicBlock::Create(SI->getContext(), "select.false",<br>
> + EndBlock->getParent(), EndBlock);<br>
> + BranchInst::Create(EndBlock, FalseBlock);<br>
> + }<br>
><br>
> // Insert the real conditional branch based on the original condition.<br>
> - BranchInst::Create(NextBlock, SmallBlock, SI->getCondition(), SI);<br>
> + // If we did not create a new block for one of the 'true' or 'false' paths<br>
> + // of the condition, it means that side of the branch goes to the end block<br>
> + // directly and the path originates from the start block from the point of<br>
> + // view of the new PHI.<br>
> + if (TrueBlock == nullptr) {<br>
> + BranchInst::Create(EndBlock, FalseBlock, SI->getCondition(), SI);<br>
> + TrueBlock = StartBlock;<br>
> + } else if (FalseBlock == nullptr) {<br>
> + BranchInst::Create(TrueBlock, EndBlock, SI->getCondition(), SI);<br>
> + FalseBlock = StartBlock;<br>
> + } else {<br>
> + BranchInst::Create(TrueBlock, FalseBlock, SI->getCondition(), SI);<br>
> + }<br>
><br>
> // The select itself is replaced with a PHI Node.<br>
> - PHINode *PN = PHINode::Create(SI->getType(), 2, "", &NextBlock->front());<br>
> + PHINode *PN = PHINode::Create(SI->getType(), 2, "", &EndBlock->front());<br>
> PN->takeName(SI);<br>
> - PN->addIncoming(SI->getTrueValue(), StartBlock);<br>
> - PN->addIncoming(SI->getFalseValue(), SmallBlock);<br>
> + PN->addIncoming(SI->getTrueValue(), TrueBlock);<br>
> + PN->addIncoming(SI->getFalseValue(), FalseBlock);<br>
> +<br>
> SI->replaceAllUsesWith(PN);<br>
> SI->eraseFromParent();<br>
><br>
><br>
> Added: llvm/trunk/test/Transforms/CodeGenPrepare/select.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/CodeGenPrepare/select.ll?rev=250527&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/CodeGenPrepare/select.ll?rev=250527&view=auto</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/CodeGenPrepare/select.ll (added)<br>
> +++ llvm/trunk/test/Transforms/CodeGenPrepare/select.ll Fri Oct 16 11:54:30 2015<br>
> @@ -0,0 +1,114 @@<br>
> +; RUN: opt -codegenprepare -S < %s | FileCheck %s<br>
> +<br>
> +target triple = "x86_64-unknown-unknown"<br>
> +<br>
> +; Nothing to sink here, but this gets converted to a branch to<br>
> +; avoid stalling an out-of-order CPU on a predictable branch.<br>
> +<br>
> +define i32 @no_sink(double %a, double* %b, i32 %x, i32 %y) {<br>
> +entry:<br>
> + %load = load double, double* %b, align 8<br>
> + %cmp = fcmp olt double %load, %a<br>
> + %sel = select i1 %cmp, i32 %x, i32 %y<br>
> + ret i32 %sel<br>
> +<br>
> +; CHECK-LABEL: @no_sink(<br>
> +; CHECK: %load = load double, double* %b, align 8<br>
> +; CHECK: %cmp = fcmp olt double %load, %a<br>
> +; CHECK: br i1 %cmp, label %select.end, label %select.false<br>
> +; CHECK: select.false:<br>
> +; CHECK: br label %select.end<br>
> +; CHECK: select.end:<br>
> +; CHECK: %sel = phi i32 [ %x, %entry ], [ %y, %select.false ]<br>
> +; CHECK: ret i32 %sel<br>
> +}<br>
> +<br>
> +<br>
> +; An 'fdiv' is expensive, so sink it rather than speculatively execute it.<br>
> +<br>
> +define float @fdiv_true_sink(float %a, float %b) {<br>
> +entry:<br>
> + %div = fdiv float %a, %b<br>
> + %cmp = fcmp ogt float %a, 1.0<br>
> + %sel = select i1 %cmp, float %div, float 2.0<br>
> + ret float %sel<br>
> +<br>
> +; CHECK-LABEL: @fdiv_true_sink(<br>
> +; CHECK: %cmp = fcmp ogt float %a, 1.0<br>
> +; CHECK: br i1 %cmp, label %select.true.sink, label %select.end<br>
> +; CHECK: select.true.sink:<br>
> +; CHECK: %div = fdiv float %a, %b<br>
> +; CHECK: br label %select.end<br>
> +; CHECK: select.end:<br>
> +; CHECK: %sel = phi float [ %div, %select.true.sink ], [ 2.000000e+00, %entry ]<br>
> +; CHECK: ret float %sel<br>
> +}<br>
> +<br>
> +define float @fdiv_false_sink(float %a, float %b) {<br>
> +entry:<br>
> + %div = fdiv float %a, %b<br>
> + %cmp = fcmp ogt float %a, 3.0<br>
> + %sel = select i1 %cmp, float 4.0, float %div<br>
> + ret float %sel<br>
> +<br>
> +; CHECK-LABEL: @fdiv_false_sink(<br>
> +; CHECK: %cmp = fcmp ogt float %a, 3.0<br>
> +; CHECK: br i1 %cmp, label %select.end, label %select.false.sink<br>
> +; CHECK: select.false.sink:<br>
> +; CHECK: %div = fdiv float %a, %b<br>
> +; CHECK: br label %select.end<br>
> +; CHECK: select.end:<br>
> +; CHECK: %sel = phi float [ 4.000000e+00, %entry ], [ %div, %select.false.sink ]<br>
> +; CHECK: ret float %sel<br>
> +}<br>
> +<br>
> +define float @fdiv_both_sink(float %a, float %b) {<br>
> +entry:<br>
> + %div1 = fdiv float %a, %b<br>
> + %div2 = fdiv float %b, %a<br>
> + %cmp = fcmp ogt float %a, 5.0<br>
> + %sel = select i1 %cmp, float %div1, float %div2<br>
> + ret float %sel<br>
> +<br>
> +; CHECK-LABEL: @fdiv_both_sink(<br>
> +; CHECK: %cmp = fcmp ogt float %a, 5.0<br>
> +; CHECK: br i1 %cmp, label %select.true.sink, label %select.false.sink<br>
> +; CHECK: select.true.sink:<br>
> +; CHECK: %div1 = fdiv float %a, %b<br>
> +; CHECK: br label %select.end<br>
> +; CHECK: select.false.sink:<br>
> +; CHECK: %div2 = fdiv float %b, %a<br>
> +; CHECK: br label %select.end<br>
> +; CHECK: select.end:<br>
> +; CHECK: %sel = phi float [ %div1, %select.true.sink ], [ %div2, %select.false.sink ]<br>
> +; CHECK: ret float %sel<br>
> +}<br>
> +<br>
> +; An 'fadd' is not too expensive, so it's ok to speculate.<br>
> +<br>
> +define float @fadd_no_sink(float %a, float %b) {<br>
> + %add = fadd float %a, %b<br>
> + %cmp = fcmp ogt float 6.0, %a<br>
> + %sel = select i1 %cmp, float %add, float 7.0<br>
> + ret float %sel<br>
> +<br>
> +; CHECK-LABEL: @fadd_no_sink(<br>
> +; CHECK: %sel = select i1 %cmp, float %add, float 7.0<br>
> +}<br>
> +<br>
> +; Possible enhancement: sinkability is only calculated with the direct<br>
> +; operand of the select, so we don't try to sink this. The fdiv cost is not<br>
> +; taken into account.<br>
> +<br>
> +define float @fdiv_no_sink(float %a, float %b) {<br>
> +entry:<br>
> + %div = fdiv float %a, %b<br>
> + %add = fadd float %div, %b<br>
> + %cmp = fcmp ogt float %a, 1.0<br>
> + %sel = select i1 %cmp, float %add, float 8.0<br>
> + ret float %sel<br>
> +<br>
> +; CHECK-LABEL: @fdiv_no_sink(<br>
> +; CHECK: %sel = select i1 %cmp, float %add, float 8.0<br>
> +}<br>
> +<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>