Index: llvm/lib/Transforms/IPO/Attributor.cpp =================================================================== --- llvm/lib/Transforms/IPO/Attributor.cpp +++ llvm/lib/Transforms/IPO/Attributor.cpp @@ -1996,16 +1996,37 @@ // TODO: We should not only check instructions that access memory // through a pointer (i.e. also branches etc.) ChangeStatus updateImpl(Attributor &A) override { - const size_t PrevSize = NoUBMemAccessInsts.size(); + const size_t PrevSize = NoUBInsts.size(); - auto InspectMemAccessInstForUB = [&](Instruction &I) { + auto InspectInstForUB = [&](Instruction &I) { // Skip instructions that are already saved. - if (NoUBMemAccessInsts.count(&I) || UBMemAccessInsts.count(&I)) + if (NoUBInsts.count(&I) || UBInsts.count(&I)) return true; - // `InspectMemAccessInstForUB` is only called on instructions - // for which getPointerOperand() should give us their - // pointer operand unless they're volatile. + // A conditional branch instruction is considered UB if it has `undef` + // condition. + // TODO: Right now, this is a very simplistic check. It only + // identifies UB in cases like: br i1 undef, ... + // This should be better when AAValueSimplfiy gets improved. + if (auto BrInst = dyn_cast(&I)) { + if (BrInst->isUnconditional()) + return true; + const Value *Cond = BrInst->getCondition(); + const auto &ValueSimplifyAA = + A.getAAFor(*this, IRPosition::value(*Cond)); + Optional SimplifiedV = + ValueSimplifyAA.getAssumedSimplifiedValue(A); + if (SimplifiedV.hasValue() && isa(SimplifiedV.getValue())) + UBInsts.insert(&I); + else + NoUBInsts.insert(&I); + return true; + } + + // If we reach here, we know we have an instruction + // that accesses memory through a pointer operand, + // for which getPointerOperand() should give it to us, + // it is volatile. const Value *PtrOp = getPointerOperand(&I); if (!PtrOp) return true; @@ -2014,7 +2035,7 @@ // only if the pointer has constant null value. // TODO: Expand it to not only check constant values. if (!isa(PtrOp)) { - NoUBMemAccessInsts.insert(&I); + NoUBInsts.insert(&I); return true; } const Type *PtrTy = PtrOp->getType(); @@ -2026,29 +2047,29 @@ // A memory access using constant null pointer is only considered UB // if null pointer is _not_ defined for the target platform. if (!llvm::NullPointerIsDefined(F, PtrTy->getPointerAddressSpace())) - UBMemAccessInsts.insert(&I); + UBInsts.insert(&I); else - NoUBMemAccessInsts.insert(&I); + NoUBInsts.insert(&I); return true; }; - A.checkForAllInstructions(InspectMemAccessInstForUB, *this, + A.checkForAllInstructions(InspectInstForUB, *this, {Instruction::Load, Instruction::Store, Instruction::AtomicCmpXchg, - Instruction::AtomicRMW}); - if (PrevSize != NoUBMemAccessInsts.size()) + Instruction::AtomicRMW, Instruction::Br}); + if (PrevSize != NoUBInsts.size()) return ChangeStatus::CHANGED; return ChangeStatus::UNCHANGED; } bool isAssumedToCauseUB(Instruction *I) const override { - return UBMemAccessInsts.count(I); + return UBInsts.count(I); } ChangeStatus manifest(Attributor &A) override { - if (!UBMemAccessInsts.size()) + if (!UBInsts.size()) return ChangeStatus::UNCHANGED; - for (Instruction *I : UBMemAccessInsts) + for (Instruction *I : UBInsts) changeToUnreachable(I, /* UseLLVMTrap */ false); return ChangeStatus::CHANGED; } @@ -2059,21 +2080,20 @@ } protected: - // A set of all the (live) memory accessing instructions that _are_ assumed to + // A set of all the (live) instructions that _are_ assumed to // cause UB. - SmallPtrSet UBMemAccessInsts; + SmallPtrSet UBInsts; private: - // A set of all the (live) memory accessing instructions - // that are _not_ assumed to cause UB. + // A set of all the (live) instructions that are _not_ assumed to cause UB. // Note: The correctness of the procedure depends on the fact that this // set stops changing after some point. "Change" here means that the size // of the set changes. The size of this set is monotonically increasing - // (we only add items to it) and is upper bounded by the number of memory - // accessing instructions in the processed function (we can never save more + // (we only add items to it) and is upper bounded by the number of + // instructions in the processed function (we can never save more // elements in this set than this number). Hence, the size of this set, at // some point, will stop increasing, effectively reaching a fixpoint. - SmallPtrSet NoUBMemAccessInsts; + SmallPtrSet NoUBInsts; }; struct AAUndefinedBehaviorFunction final : AAUndefinedBehaviorImpl { @@ -2085,7 +2105,7 @@ STATS_DECL(UndefinedBehaviorInstruction, Instruction, "Number of instructions known to have UB"); BUILD_STAT_NAME(UndefinedBehaviorInstruction, Instruction) += - UBMemAccessInsts.size(); + UBInsts.size(); } }; @@ -5580,6 +5600,7 @@ // The alignment of a pointer is interesting for stores. case Instruction::Call: case Instruction::CallBr: + case Instruction::Br: case Instruction::Invoke: case Instruction::CleanupRet: case Instruction::CatchSwitch: Index: llvm/test/Transforms/Attributor/undefined_behavior.ll =================================================================== --- llvm/test/Transforms/Attributor/undefined_behavior.ll +++ llvm/test/Transforms/Attributor/undefined_behavior.ll @@ -8,9 +8,10 @@ ; -- Load tests -- -; ATTRIBUTOR-LABEL: define void @load_wholly_unreachable() define void @load_wholly_unreachable() { -; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR-LABEL: @load_wholly_unreachable( +; ATTRIBUTOR-NEXT: unreachable +; %a = load i32, i32* null ret void } @@ -144,3 +145,46 @@ %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic ret void } + +; Note: The unreachable on %t and %e is _not_ from AAUndefinedBehavior +define i32 @cond_br_on_undef() { +; ATTRIBUTOR-LABEL: @cond_br_on_undef( +; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR: t: +; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR: e: +; ATTRIBUTOR-NEXT: unreachable +; + + br i1 undef, label %t, label %e +t: + ret i32 1 +e: + ret i32 2 +} + +define void @cond_br_on_undef2(i1 %cond) { +; ATTRIBUTOR-LABEL: @cond_br_on_undef2( +; ATTRIBUTOR-NEXT: br i1 [[COND:%.*]], label [[T1:%.*]], label [[E1:%.*]] +; ATTRIBUTOR: t1: +; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR: t2: +; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR: e2: +; ATTRIBUTOR-NEXT: unreachable +; ATTRIBUTOR: e1: +; ATTRIBUTOR-NEXT: ret void +; + + ; Valid branch - verify that this is not converted + ; to unreachable. + br i1 %cond, label %t1, label %e1 +t1: + br i1 undef, label %t2, label %e2 +t2: + ret void +e2: + ret void +e1: + ret void +}