[llvm] r204537 - [Constant Hoisting] Fix multiple entries for the same basic block in PHI nodes.
Eric Christopher
echristo at gmail.com
Thu Apr 3 02:53:33 PDT 2014
On Fri, Mar 21, 2014 at 6:49 PM, Juergen Ributzka <juergen at apple.com> wrote:
> Author: ributzka
> Date: Fri Mar 21 20:49:27 2014
> New Revision: 204537
>
> URL: http://llvm.org/viewvc/llvm-project?rev=204537&view=rev
> Log:
> [Constant Hoisting] Fix multiple entries for the same basic block in PHI nodes.
>
> A PHI node usually has only one value/basic block pair per incoming basic block.
> In the case of a switch statement it is possible that a following PHI node may
> have more than one such pair per incoming basic block. E.g.:
> %0 = phi i64 [ 123456, %case2 ], [ 654321, %Entry ], [ 654321, %Entry ]
> This is valid and the verfier doesn't complain, because both values are the
> same.
>
> Constant hoisting materializes the constant for each operand separately and the
> value is still the same, but the variable names have changed. As a result the
> verfier can't recognize anymore that they are the same value and complains.
>
> This fix adds special update code for PHI node in constant hoisting to prevent
> this corner case.
Fascinating. Interesting fix. :)
-eric
>
> This fixes <rdar://problem/16394449>
>
> Modified:
> llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp
> llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp?rev=204537&r1=204536&r2=204537&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp Fri Mar 21 20:49:27 2014
> @@ -456,6 +456,34 @@ void ConstantHoisting::findBaseConstants
> findAndMakeBaseConstant(MinValItr, ConstCandVec.end());
> }
>
> +/// \brief Updates the operand at Idx in instruction Inst with the result of
> +/// instruction Mat. If the instruction is a PHI node then special
> +/// handling for duplicate values form the same incomming basic block is
> +/// required.
> +/// \return The update will always succeed, but the return value indicated if
> +/// Mat was used for the update or not.
> +static bool updateOperand(Instruction *Inst, unsigned Idx, Instruction *Mat) {
> + if (auto PHI = dyn_cast<PHINode>(Inst)) {
> + // Check if any previous operand of the PHI node has the same incoming basic
> + // block. This is a very odd case that happens when the incoming basic block
> + // has a switch statement. In this case use the same value as the previous
> + // operand(s), otherwise we will fail verification due to different values.
> + // The values are actually the same, but the variable names are different
> + // and the verifier doesn't like that.
> + BasicBlock *IncomingBB = PHI->getIncomingBlock(Idx);
> + for (unsigned i = 0; i < Idx; ++i) {
> + if (PHI->getIncomingBlock(i) == IncomingBB) {
> + Value *IncomingVal = PHI->getIncomingValue(i);
> + Inst->setOperand(Idx, IncomingVal);
> + return false;
> + }
> + }
> + }
> +
> + Inst->setOperand(Idx, Mat);
> + return true;
> +}
> +
> /// \brief Emit materialization code for all rebased constants and update their
> /// users.
> void ConstantHoisting::emitBaseConstants(Instruction *Base, Constant *Offset,
> @@ -477,7 +505,8 @@ void ConstantHoisting::emitBaseConstants
> // Visit constant integer.
> if (isa<ConstantInt>(Opnd)) {
> DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
> - ConstUser.Inst->setOperand(ConstUser.OpndIdx, Mat);
> + if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat) && Offset)
> + Mat->eraseFromParent();
> DEBUG(dbgs() << "To : " << *ConstUser.Inst << '\n');
> return;
> }
> @@ -499,7 +528,7 @@ void ConstantHoisting::emitBaseConstants
> }
>
> DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
> - ConstUser.Inst->setOperand(ConstUser.OpndIdx, ClonedCastInst);
> + updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ClonedCastInst);
> DEBUG(dbgs() << "To : " << *ConstUser.Inst << '\n');
> return;
> }
> @@ -517,7 +546,11 @@ void ConstantHoisting::emitBaseConstants
> DEBUG(dbgs() << "Create instruction: " << *ConstExprInst << '\n'
> << "From : " << *ConstExpr << '\n');
> DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
> - ConstUser.Inst->setOperand(ConstUser.OpndIdx, ConstExprInst);
> + if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ConstExprInst)) {
> + ConstExprInst->eraseFromParent();
> + if (Offset)
> + Mat->eraseFromParent();
> + }
> DEBUG(dbgs() << "To : " << *ConstUser.Inst << '\n');
> return;
> }
>
> Modified: llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll?rev=204537&r1=204536&r2=204537&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll (original)
> +++ llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll Fri Mar 21 20:49:27 2014
> @@ -68,3 +68,49 @@ if.then8:
> if.end9: ; preds = %if.then8, %if.end3
> ret i32 undef
> }
> +
> +; <rdar://problem/16394449>
> +define i64 @switch_test1(i64 %a) {
> +; CHECK-LABEL: @switch_test1
> +; CHECK: %0 = phi i64 [ %const, %case2 ], [ %const_mat, %Entry ], [ %const_mat, %Entry ]
> +Entry:
> + %sel = add i64 %a, 4519019440
> + switch i64 %sel, label %fail [
> + i64 462, label %continuation
> + i64 449, label %case2
> + i64 443, label %continuation
> + ]
> +
> +case2:
> + br label %continuation
> +
> +continuation:
> + %0 = phi i64 [ 4519019440, %case2 ], [ 4519019460, %Entry ], [ 4519019460, %Entry ]
> + ret i64 0;
> +
> +fail:
> + ret i64 -1;
> +}
> +
> +define i64 @switch_test2(i64 %a) {
> +; CHECK-LABEL: @switch_test2
> +; CHECK: %2 = phi i64* [ %1, %case2 ], [ %0, %Entry ], [ %0, %Entry ]
> +Entry:
> + %sel = add i64 %a, 4519019440
> + switch i64 %sel, label %fail [
> + i64 462, label %continuation
> + i64 449, label %case2
> + i64 443, label %continuation
> + ]
> +
> +case2:
> + br label %continuation
> +
> +continuation:
> + %0 = phi i64* [ inttoptr(i64 4519019440 to i64*), %case2 ], [ inttoptr(i64 4519019460 to i64*), %Entry ], [ inttoptr(i64 4519019460 to i64*), %Entry ]
> + ret i64 0;
> +
> +fail:
> + ret i64 -1;
> +}
> +
>
>
> _______________________________________________
> 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