[llvm] 5732f56 - [Attributor] UB Attribute now handles all instructions that access memory through a pointer
Johannes Doerfert via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 24 18:46:25 PST 2019
Author: Johannes Doerfert
Date: 2019-12-24T19:25:08-06:00
New Revision: 5732f56bbd28be6cab976e1df0d87ac5ffae7fcd
URL: https://github.com/llvm/llvm-project/commit/5732f56bbd28be6cab976e1df0d87ac5ffae7fcd
DIFF: https://github.com/llvm/llvm-project/commit/5732f56bbd28be6cab976e1df0d87ac5ffae7fcd.diff
LOG: [Attributor] UB Attribute now handles all instructions that access memory through a pointer
Summary:
Follow-up on: https://reviews.llvm.org/D71435
We basically use `checkForAllInstructions` to loop through all the instructions in a function that access memory through a pointer: load, store, atomicrmw, atomiccmpxchg
Note that we can now use the `getPointerOperand()` that gets us the pointer operand for an instruction that belongs to the aforementioned set.
Question: This function returns `nullptr` if the instruction is `volatile`. Why?
Guess: Because if it is volatile, we don't want to do any transformation to it.
Another subtle point is that I had to add AtomicRMW, AtomicCmpXchg to `initializeInformationCache()`. Following `checkAllInstructions()` path, that
seemed the most reasonable place to add it and correct the fact that these instructions were ignored (they were not in `OpcodeInstMap` etc.). Is that ok?
Reviewers: jdoerfert, sstefan1
Reviewed By: jdoerfert, sstefan1
Subscribers: hiraditya, jfb, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D71787
Added:
Modified:
llvm/lib/Transforms/IPO/Attributor.cpp
llvm/test/Transforms/Attributor/undefined_behavior.ll
Removed:
################################################################################
diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp
index 1bb3b41c6b6c..b08bc1fda65c 100644
--- a/llvm/lib/Transforms/IPO/Attributor.cpp
+++ b/llvm/lib/Transforms/IPO/Attributor.cpp
@@ -1993,53 +1993,62 @@ struct AAUndefinedBehaviorImpl : public AAUndefinedBehavior {
AAUndefinedBehaviorImpl(const IRPosition &IRP) : AAUndefinedBehavior(IRP) {}
/// See AbstractAttribute::updateImpl(...).
+ // TODO: We should not only check instructions that access memory
+ // through a pointer (i.e. also branches etc.)
ChangeStatus updateImpl(Attributor &A) override {
- size_t PrevSize = NoUBLoads.size();
+ const size_t PrevSize = NoUBMemAccessInsts.size();
- // TODO: We should not only check for load instructions.
- auto InspectLoadForUB = [&](Instruction &I) {
+ auto InspectMemAccessInstForUB = [&](Instruction &I) {
// Skip instructions that are already saved.
- if (NoUBLoads.count(&I) || UBLoads.count(&I))
+ if (NoUBMemAccessInsts.count(&I) || UBMemAccessInsts.count(&I))
return true;
- Value *PtrOp = cast<LoadInst>(&I)->getPointerOperand();
+ // `InspectMemAccessInstForUB` is only called on instructions
+ // for which getPointerOperand() should give us their
+ // pointer operand unless they're volatile.
+ const Value *PtrOp = getPointerOperand(&I);
+ if (!PtrOp)
+ return true;
- // A load is considered UB only if it dereferences a constant
- // null pointer.
+ // A memory access through a pointer is considered UB
+ // only if the pointer has constant null value.
+ // TODO: Expand it to not only check constant values.
if (!isa<ConstantPointerNull>(PtrOp)) {
- NoUBLoads.insert(&I);
+ NoUBMemAccessInsts.insert(&I);
return true;
}
- Type *PtrTy = PtrOp->getType();
+ const Type *PtrTy = PtrOp->getType();
- // Because we only consider loads inside functions,
+ // Because we only consider instructions inside functions,
// assume that a parent function exists.
const Function *F = I.getFunction();
- // A dereference on constant null is only considered UB
- // if null dereference is _not_ defined for the target platform.
- // TODO: Expand it to not only check constant values.
+ // 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()))
- UBLoads.insert(&I);
+ UBMemAccessInsts.insert(&I);
else
- NoUBLoads.insert(&I);
+ NoUBMemAccessInsts.insert(&I);
return true;
};
- A.checkForAllInstructions(InspectLoadForUB, *this, {Instruction::Load});
- if (PrevSize != NoUBLoads.size())
+ A.checkForAllInstructions(InspectMemAccessInstForUB, *this,
+ {Instruction::Load, Instruction::Store,
+ Instruction::AtomicCmpXchg,
+ Instruction::AtomicRMW});
+ if (PrevSize != NoUBMemAccessInsts.size())
return ChangeStatus::CHANGED;
return ChangeStatus::UNCHANGED;
}
bool isAssumedToCauseUB(Instruction *I) const override {
- return UBLoads.count(I);
+ return UBMemAccessInsts.count(I);
}
ChangeStatus manifest(Attributor &A) override {
- if (!UBLoads.size())
+ if (!UBMemAccessInsts.size())
return ChangeStatus::UNCHANGED;
- for (Instruction *I : UBLoads)
+ for (Instruction *I : UBMemAccessInsts)
changeToUnreachable(I, /* UseLLVMTrap */ false);
return ChangeStatus::CHANGED;
}
@@ -2050,20 +2059,21 @@ struct AAUndefinedBehaviorImpl : public AAUndefinedBehavior {
}
protected:
- // A set of all the (live) load instructions that _are_ assumed to cause UB.
- SmallPtrSet<Instruction *, 8> UBLoads;
+ // A set of all the (live) memory accessing instructions that _are_ assumed to
+ // cause UB.
+ SmallPtrSet<Instruction *, 8> UBMemAccessInsts;
private:
- // A set of all the (live) load instructions that are _not_ assumed to cause
- // UB.
+ // A set of all the (live) memory accessing 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 load
- // 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<Instruction *, 8> NoUBLoads;
+ // (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
+ // elements in this set than this number). Hence, the size of this set, at
+ // some point, will stop increasing, effectively reaching a fixpoint.
+ SmallPtrSet<Instruction *, 8> NoUBMemAccessInsts;
};
struct AAUndefinedBehaviorFunction final : AAUndefinedBehaviorImpl {
@@ -2075,7 +2085,7 @@ struct AAUndefinedBehaviorFunction final : AAUndefinedBehaviorImpl {
STATS_DECL(UndefinedBehaviorInstruction, Instruction,
"Number of instructions known to have UB");
BUILD_STAT_NAME(UndefinedBehaviorInstruction, Instruction) +=
- UBLoads.size();
+ UBMemAccessInsts.size();
}
};
@@ -5573,6 +5583,8 @@ void Attributor::initializeInformationCache(Function &F) {
case Instruction::Invoke:
case Instruction::CleanupRet:
case Instruction::CatchSwitch:
+ case Instruction::AtomicRMW:
+ case Instruction::AtomicCmpXchg:
case Instruction::Resume:
case Instruction::Ret:
IsInterestingOpcode = true;
@@ -5812,9 +5824,9 @@ raw_ostream &llvm::operator<<(raw_ostream &OS, const IRPosition &Pos) {
}
template <typename base_ty, base_ty BestState, base_ty WorstState>
-raw_ostream &
-llvm::operator<<(raw_ostream &OS,
- const IntegerStateBase<base_ty, BestState, WorstState> &S) {
+raw_ostream &llvm::
+operator<<(raw_ostream &OS,
+ const IntegerStateBase<base_ty, BestState, WorstState> &S) {
return OS << "(" << S.getKnown() << "-" << S.getAssumed() << ")"
<< static_cast<const AbstractState &>(S);
}
diff --git a/llvm/test/Transforms/Attributor/undefined_behavior.ll b/llvm/test/Transforms/Attributor/undefined_behavior.ll
index 873f16a50bec..40fe11dc13f6 100644
--- a/llvm/test/Transforms/Attributor/undefined_behavior.ll
+++ b/llvm/test/Transforms/Attributor/undefined_behavior.ll
@@ -6,22 +6,23 @@ target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
; We want to verify that whenever undefined behavior is assumed, the code becomes unreachable.
; We use FIXME's to indicate problems and missing attributes.
-; ATTRIBUTOR: define void @wholly_unreachable()
+; -- Load tests --
+
+; ATTRIBUTOR-LABEL: define void @load_wholly_unreachable()
+define void @load_wholly_unreachable() {
; ATTRIBUTOR-NEXT: unreachable
-define void @wholly_unreachable() {
%a = load i32, i32* null
ret void
}
-; ATTRIBUTOR: define void @single_bb_unreachable(i1 %cond)
-; ATTRIBUTOR-NEXT: br i1 %cond, label %t, label %e
-; ATTRIBUTOR-EMPTY:
-; ATTRIBUTOR-NEXT: t:
-; ATTRIBUTOR-NEXT: unreachable
-; ATTRIBUTOR-EMPTY:
-; ATTRIBUTOR-NEXT: e:
-; ATTRIBUTOR-NEXT: ret void
-define void @single_bb_unreachable(i1 %cond) {
+define void @load_single_bb_unreachable(i1 %cond) {
+; ATTRIBUTOR-LABEL: @load_single_bb_unreachable(
+; ATTRIBUTOR-NEXT: br i1 [[COND:%.*]], label [[T:%.*]], label [[E:%.*]]
+; ATTRIBUTOR: t:
+; ATTRIBUTOR-NEXT: unreachable
+; ATTRIBUTOR: e:
+; ATTRIBUTOR-NEXT: ret void
+;
br i1 %cond, label %t, label %e
t:
%b = load i32, i32* null
@@ -30,9 +31,116 @@ e:
ret void
}
-; ATTRIBUTOR: define void @null_pointer_is_defined()
-; ATTRIBUTOR-NEXT: %a = load i32, i32* null
-define void @null_pointer_is_defined() "null-pointer-is-valid"="true" {
+define void @load_null_pointer_is_defined() "null-pointer-is-valid"="true" {
+; ATTRIBUTOR-LABEL: @load_null_pointer_is_defined(
+; ATTRIBUTOR-NEXT: [[A:%.*]] = load i32, i32* null
+; ATTRIBUTOR-NEXT: ret void
+;
%a = load i32, i32* null
ret void
}
+
+; -- Store tests --
+
+define void @store_wholly_unreachable() {
+; ATTRIBUTOR-LABEL: @store_wholly_unreachable(
+; ATTRIBUTOR-NEXT: unreachable
+;
+ store i32 5, i32* null
+ ret void
+}
+
+define void @store_single_bb_unreachable(i1 %cond) {
+; ATTRIBUTOR-LABEL: @store_single_bb_unreachable(
+; ATTRIBUTOR-NEXT: br i1 [[COND:%.*]], label [[T:%.*]], label [[E:%.*]]
+; ATTRIBUTOR: t:
+; ATTRIBUTOR-NEXT: unreachable
+; ATTRIBUTOR: e:
+; ATTRIBUTOR-NEXT: ret void
+;
+ br i1 %cond, label %t, label %e
+t:
+ store i32 5, i32* null
+ br label %e
+e:
+ ret void
+}
+
+define void @store_null_pointer_is_defined() "null-pointer-is-valid"="true" {
+; ATTRIBUTOR-LABEL: @store_null_pointer_is_defined(
+; ATTRIBUTOR-NEXT: store i32 5, i32* null
+; ATTRIBUTOR-NEXT: ret void
+;
+ store i32 5, i32* null
+ ret void
+}
+
+; -- AtomicRMW tests --
+
+define void @atomicrmw_wholly_unreachable() {
+; ATTRIBUTOR-LABEL: @atomicrmw_wholly_unreachable(
+; ATTRIBUTOR-NEXT: unreachable
+;
+ %a = atomicrmw add i32* null, i32 1 acquire
+ ret void
+}
+
+define void @atomicrmw_single_bb_unreachable(i1 %cond) {
+; ATTRIBUTOR-LABEL: @atomicrmw_single_bb_unreachable(
+; ATTRIBUTOR-NEXT: br i1 [[COND:%.*]], label [[T:%.*]], label [[E:%.*]]
+; ATTRIBUTOR: t:
+; ATTRIBUTOR-NEXT: unreachable
+; ATTRIBUTOR: e:
+; ATTRIBUTOR-NEXT: ret void
+;
+ br i1 %cond, label %t, label %e
+t:
+ %a = atomicrmw add i32* null, i32 1 acquire
+ br label %e
+e:
+ ret void
+}
+
+define void @atomicrmw_null_pointer_is_defined() "null-pointer-is-valid"="true" {
+; ATTRIBUTOR-LABEL: @atomicrmw_null_pointer_is_defined(
+; ATTRIBUTOR-NEXT: [[A:%.*]] = atomicrmw add i32* null, i32 1 acquire
+; ATTRIBUTOR-NEXT: ret void
+;
+ %a = atomicrmw add i32* null, i32 1 acquire
+ ret void
+}
+
+; -- AtomicCmpXchg tests --
+
+define void @atomiccmpxchg_wholly_unreachable() {
+; ATTRIBUTOR-LABEL: @atomiccmpxchg_wholly_unreachable(
+; ATTRIBUTOR-NEXT: unreachable
+;
+ %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
+ ret void
+}
+
+define void @atomiccmpxchg_single_bb_unreachable(i1 %cond) {
+; ATTRIBUTOR-LABEL: @atomiccmpxchg_single_bb_unreachable(
+; ATTRIBUTOR-NEXT: br i1 [[COND:%.*]], label [[T:%.*]], label [[E:%.*]]
+; ATTRIBUTOR: t:
+; ATTRIBUTOR-NEXT: unreachable
+; ATTRIBUTOR: e:
+; ATTRIBUTOR-NEXT: ret void
+;
+ br i1 %cond, label %t, label %e
+t:
+ %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
+ br label %e
+e:
+ ret void
+}
+
+define void @atomiccmpxchg_null_pointer_is_defined() "null-pointer-is-valid"="true" {
+; ATTRIBUTOR-LABEL: @atomiccmpxchg_null_pointer_is_defined(
+; ATTRIBUTOR-NEXT: [[A:%.*]] = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
+; ATTRIBUTOR-NEXT: ret void
+;
+ %a = cmpxchg i32* null, i32 2, i32 3 acq_rel monotonic
+ ret void
+}
More information about the llvm-commits
mailing list