[Clang] Add fake use emission to Clang with -fextend-lifetimes #110102
Conversation
SLTozer
requested review from
asl,
adrian-prantl,
dwblaikie,
pogo59 and
wolfy1961
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Stephen Tozer (SLTozer) ChangesFollowing the previous patch which adds the "extend lifetimes" flag without (almost) any functionality, this patch adds the real feature by allowing Clang to emit fake uses. These are emitted as a new form of cleanup, set for variable addresses, which just emits a fake use intrinsic when the variable falls out of scope. The code for achieving this is simple, with most of the logic centered on determining whether to emit a fake use for a given address, and on ensuring that fake uses are ignored in a few cases. All code originally written by @wolfy1961, while I'll be handling the review. Patch is 26.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/110102.diff 21 Files Affected:
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 4ae981e4013e9c..13c556a4e20a13 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3566,16 +3566,26 @@ static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
llvm::BasicBlock *IP = CGF.Builder.GetInsertBlock();
if (IP->empty()) return nullptr;
- // Look at directly preceding instruction, skipping bitcasts and lifetime
- // markers.
- for (llvm::Instruction &I : make_range(IP->rbegin(), IP->rend())) {
- if (isa<llvm::BitCastInst>(&I))
- continue;
- if (auto *II = dyn_cast<llvm::IntrinsicInst>(&I))
- if (II->getIntrinsicID() == llvm::Intrinsic::lifetime_end)
- continue;
-
- return GetStoreIfValid(&I);
+ // Look at directly preceding instruction, skipping bitcasts, lifetime
+ // markers, and fake uses and their operands.
+ const llvm::Instruction *LoadIntoFakeUse = nullptr;
+ for (llvm::Instruction &I : make_range(IP->rbegin(), IP->rend())) {
+ // Ignore instructions that are just loads for fake uses; the load should
+ // immediately precede the fake use, so we only need to remember the
+ // operand for the last fake use seen.
+ if (LoadIntoFakeUse == &I)
+ continue;
+ if (isa<llvm::BitCastInst>(&I))
+ continue;
+ if (auto *II = dyn_cast<llvm::IntrinsicInst>(&I)) {
+ if (II->getIntrinsicID() == llvm::Intrinsic::lifetime_end)
+ continue;
+
+ if (II->getIntrinsicID() == llvm::Intrinsic::fake_use) {
+ LoadIntoFakeUse = dyn_cast<llvm::Instruction>(II->getArgOperand(0));
+ continue;
+ }
+ } return GetStoreIfValid(&I);
}
return nullptr;
}
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index 5d253c92a38a81..82532e182bebbd 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -112,11 +112,11 @@ void EHScopeStack::deallocate(size_t Size) {
StartOfData += llvm::alignTo(Size, ScopeStackAlignment);
}
-bool EHScopeStack::containsOnlyLifetimeMarkers(
+bool EHScopeStack::containsOnlyNoopCleanups(
EHScopeStack::stable_iterator Old) const {
for (EHScopeStack::iterator it = begin(); stabilize(it) != Old; it++) {
EHCleanupScope *cleanup = dyn_cast<EHCleanupScope>(&*it);
- if (!cleanup || !cleanup->isLifetimeMarker())
+ if (!cleanup || !(cleanup->isLifetimeMarker() || cleanup->isFakeUse()))
return false;
}
@@ -154,6 +154,7 @@ void *EHScopeStack::pushCleanup(CleanupKind Kind, size_t Size) {
bool IsNormalCleanup = Kind & NormalCleanup;
bool IsEHCleanup = Kind & EHCleanup;
bool IsLifetimeMarker = Kind & LifetimeMarker;
+ bool IsFakeUse = Kind & FakeUse;
// Per C++ [except.terminate], it is implementation-defined whether none,
// some, or all cleanups are called before std::terminate. Thus, when
@@ -176,6 +177,8 @@ void *EHScopeStack::pushCleanup(CleanupKind Kind, size_t Size) {
InnermostEHScope = stable_begin();
if (IsLifetimeMarker)
Scope->setLifetimeMarker();
+ if (IsFakeUse)
+ Scope->setFakeUse();
// With Windows -EHa, Invoke llvm.seh.scope.begin() for EHCleanup
// If exceptions are disabled/ignored and SEH is not in use, then there is no
diff --git a/clang/lib/CodeGen/CGCleanup.h b/clang/lib/CodeGen/CGCleanup.h
index c73c97146abc4d..ba78e5478ac373 100644
--- a/clang/lib/CodeGen/CGCleanup.h
+++ b/clang/lib/CodeGen/CGCleanup.h
@@ -87,6 +87,10 @@ class EHScope {
LLVM_PREFERRED_TYPE(bool)
unsigned IsLifetimeMarker : 1;
+ /// Whether this cleanup is a fake use
+ LLVM_PREFERRED_TYPE(bool)
+ unsigned IsFakeUse : 1;
+
/// Whether the normal cleanup should test the activation flag.
LLVM_PREFERRED_TYPE(bool)
unsigned TestFlagInNormalCleanup : 1;
@@ -352,6 +356,7 @@ class alignas(8) EHCleanupScope : public EHScope {
CleanupBits.IsEHCleanup = isEH;
CleanupBits.IsActive = true;
CleanupBits.IsLifetimeMarker = false;
+ CleanupBits.IsFakeUse = false;
CleanupBits.TestFlagInNormalCleanup = false;
CleanupBits.TestFlagInEHCleanup = false;
CleanupBits.CleanupSize = cleanupSize;
@@ -384,6 +389,9 @@ class alignas(8) EHCleanupScope : public EHScope {
bool isLifetimeMarker() const { return CleanupBits.IsLifetimeMarker; }
void setLifetimeMarker() { CleanupBits.IsLifetimeMarker = true; }
+ bool isFakeUse() const { return CleanupBits.IsFakeUse; }
+ void setFakeUse() { CleanupBits.IsFakeUse = true; }
+
bool hasActiveFlag() const { return ActiveFlag.isValid(); }
Address getActiveFlag() const {
return ActiveFlag;
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 563f728e29d781..7fdf18f3ca2e7b 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1353,6 +1353,14 @@ void CodeGenFunction::EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr) {
C->setDoesNotThrow();
}
+void CodeGenFunction::EmitFakeUse(Address Addr) {
+ auto NL = ApplyDebugLocation::CreateEmpty(*this);
+ llvm::Value *V = Builder.CreateLoad(Addr, "fake.use");
+ llvm::CallInst *C = Builder.CreateCall(CGM.getLLVMFakeUseFn(), {V});
+ C->setDoesNotThrow();
+ C->setTailCallKind(llvm::CallInst::TCK_NoTail);
+}
+
void CodeGenFunction::EmitAndRegisterVariableArrayDimensions(
CGDebugInfo *DI, const VarDecl &D, bool EmitDebugInfo) {
// For each dimension stores its QualType and corresponding
@@ -1412,6 +1420,39 @@ void CodeGenFunction::EmitAndRegisterVariableArrayDimensions(
}
}
+/// Return the maximum size of an aggregate for which we generate a fake use
+/// intrinsic when -fextend-lifetimes is in effect.
+static uint64_t maxFakeUseAggregateSize(const ASTContext &C) {
+ return 4 * C.getTypeSize(C.UnsignedIntTy);
+}
+
+// Helper function to determine whether a variable's or parameter's lifetime
+// should be extended.
+static bool extendLifetime(const ASTContext &Context, const Decl *FuncDecl,
+ const VarDecl &D,
+ ImplicitParamDecl *CXXABIThisDecl) {
+ // When we're not inside a valid function it is unlikely that any
+ // lifetime extension is useful.
+ if (!FuncDecl)
+ return false;
+ if (FuncDecl->isImplicit())
+ return false;
+ // Do not extend compiler-created variables except for the this pointer.
+ if (D.isImplicit() && &D != CXXABIThisDecl)
+ return false;
+ QualType Ty = D.getType();
+ // No need to extend volatiles, they have a memory location.
+ if (Ty.isVolatileQualified())
+ return false;
+ // Don't extend variables that exceed a certain size.
+ if (Context.getTypeSize(Ty) > maxFakeUseAggregateSize(Context))
+ return false;
+ // Do not extend variables in nodebug functions.
+ if (FuncDecl->hasAttr<NoDebugAttr>())
+ return false;
+ return true;
+}
+
/// EmitAutoVarAlloca - Emit the alloca and debug information for a
/// local variable. Does not emit initialization or destruction.
CodeGenFunction::AutoVarEmission
@@ -1664,6 +1705,17 @@ CodeGenFunction::EmitAutoVarAlloca(const VarDecl &D) {
emission.getOriginalAllocatedAddress(),
emission.getSizeForLifetimeMarkers());
+ // Analogous to lifetime markers, we use a 'cleanup' to emit fake.use
+ // calls for local variables. We are exempting volatile variables and
+ // non-scalars larger than 4 times the size of an unsigned int (32 bytes).
+ // Larger non-scalars are often allocated in memory and may create unnecessary
+ // overhead.
+ if (CGM.getCodeGenOpts().ExtendLifetimes) {
+ if (extendLifetime(getContext(), CurCodeDecl, D, CXXABIThisDecl))
+ EHStack.pushCleanup<FakeUse>(NormalFakeUse,
+ emission.getAllocatedAddress());
+ }
+
return emission;
}
@@ -2523,6 +2575,14 @@ llvm::Function *CodeGenModule::getLLVMLifetimeEndFn() {
return LifetimeEndFn;
}
+/// Lazily declare the @llvm.fake.use intrinsic.
+llvm::Function *CodeGenModule::getLLVMFakeUseFn() {
+ if (!FakeUseFn)
+ FakeUseFn = llvm::Intrinsic::getDeclaration(&getModule(),
+ llvm::Intrinsic::fake_use);
+ return FakeUseFn;
+}
+
namespace {
/// A cleanup to perform a release of an object at the end of a
/// function. This is used to balance out the incoming +1 of a
@@ -2716,6 +2776,15 @@ void CodeGenFunction::EmitParmDecl(const VarDecl &D, ParamValue Arg,
setAddrOfLocalVar(&D, DeclPtr);
+ // Push a FakeUse 'cleanup' object onto the EHStack for the parameter,
+ // which may be the 'this' pointer. This causes the emission of a fake.use
+ // call with the parameter as argument at the end of the function.
+ if (CGM.getCodeGenOpts().ExtendLifetimes ||
+ (CGM.getCodeGenOpts().ExtendThisPtr && &D == CXXABIThisDecl)) {
+ if (extendLifetime(getContext(), CurCodeDecl, D, CXXABIThisDecl))
+ EHStack.pushCleanup<FakeUse>(NormalFakeUse, DeclPtr);
+ }
+
// Emit debug info for param declarations in non-thunk functions.
if (CGDebugInfo *DI = getDebugInfo()) {
if (CGM.getCodeGenOpts().hasReducedDebugInfo() && !CurFuncIsThunk &&
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index eda96f3e352ce3..630a9c8db95fe5 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -403,9 +403,9 @@ void CodeGenFunction::FinishFunction(SourceLocation EndLoc) {
// important to do this before we enter the return block or return
// edges will be *really* confused.
bool HasCleanups = EHStack.stable_begin() != PrologueCleanupDepth;
- bool HasOnlyLifetimeMarkers =
- HasCleanups && EHStack.containsOnlyLifetimeMarkers(PrologueCleanupDepth);
- bool EmitRetDbgLoc = !HasCleanups || HasOnlyLifetimeMarkers;
+ bool HasOnlyNoopCleanups =
+ HasCleanups && EHStack.containsOnlyNoopCleanups(PrologueCleanupDepth);
+ bool EmitRetDbgLoc = !HasCleanups || HasOnlyNoopCleanups;
std::optional<ApplyDebugLocation> OAL;
if (HasCleanups) {
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 3e2abbd9bc1094..1525eb0f09b6e5 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -718,6 +718,20 @@ class CodeGenFunction : public CodeGenTypeCache {
}
};
+ // We are using objects of this 'cleanup' class to emit fake.use calls
+ // for -fextend-lifetimes and -fextend-this-ptr. They are placed at the end of
+ // a variable's scope analogous to lifetime markers.
+ class FakeUse final : public EHScopeStack::Cleanup {
+ Address Addr;
+
+ public:
+ FakeUse(Address addr) : Addr(addr) {}
+
+ void Emit(CodeGenFunction &CGF, Flags flags) override {
+ CGF.EmitFakeUse(Addr);
+ }
+ };
+
/// Header for data within LifetimeExtendedCleanupStack.
struct LifetimeExtendedCleanupHeader {
/// The size of the following cleanup object.
@@ -4975,6 +4989,8 @@ class CodeGenFunction : public CodeGenTypeCache {
RValue EmitAtomicExpr(AtomicExpr *E);
+ void EmitFakeUse(Address Addr);
+
//===--------------------------------------------------------------------===//
// Annotations Emission
//===--------------------------------------------------------------------===//
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index c58bb88035ca8a..71198134f17413 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -601,6 +601,9 @@ class CodeGenModule : public CodeGenTypeCache {
/// void @llvm.lifetime.end(i64 %size, i8* nocapture <ptr>)
llvm::Function *LifetimeEndFn = nullptr;
+ /// void @llvm.fake.use(...)
+ llvm::Function *FakeUseFn = nullptr;
+
std::unique_ptr<SanitizerMetadata> SanitizerMD;
llvm::MapVector<const Decl *, bool> DeferredEmptyCoverageMappingDecls;
@@ -1268,6 +1271,7 @@ class CodeGenModule : public CodeGenTypeCache {
llvm::Function *getLLVMLifetimeStartFn();
llvm::Function *getLLVMLifetimeEndFn();
+ llvm::Function *getLLVMFakeUseFn();
// Make sure that this type is translated.
void UpdateCompletedType(const TagDecl *TD);
diff --git a/clang/lib/CodeGen/EHScopeStack.h b/clang/lib/CodeGen/EHScopeStack.h
index 0c667e80bb6d8c..ed11dc2bb05d73 100644
--- a/clang/lib/CodeGen/EHScopeStack.h
+++ b/clang/lib/CodeGen/EHScopeStack.h
@@ -87,6 +87,11 @@ enum CleanupKind : unsigned {
LifetimeMarker = 0x8,
NormalEHLifetimeMarker = LifetimeMarker | NormalAndEHCleanup,
+
+ // FakeUse needs to be recognized as a special cleanup similar to lifetime
+ // markers chiefly to be ignored in most contexts.
+ FakeUse = 0x10,
+ NormalFakeUse = FakeUse | NormalCleanup,
};
/// A stack of scopes which respond to exceptions, including cleanups
@@ -352,8 +357,8 @@ class EHScopeStack {
void popTerminate();
// Returns true iff the current scope is either empty or contains only
- // lifetime markers, i.e. no real cleanup code
- bool containsOnlyLifetimeMarkers(stable_iterator Old) const;
+ // noop cleanups, i.e. lifetime markers and fake uses.
+ bool containsOnlyNoopCleanups(stable_iterator Old) const;
/// Determines whether the exception-scopes stack is empty.
bool empty() const { return StartOfData == EndOfBuffer; }
diff --git a/clang/test/CodeGen/extend-liveness1.c b/clang/test/CodeGen/extend-liveness1.c
new file mode 100644
index 00000000000000..ef2d00eb6be312
--- /dev/null
+++ b/clang/test/CodeGen/extend-liveness1.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-lifetimes -o - | FileCheck %s
+// Check that fake use calls are emitted at the correct locations, i.e.
+// at the end of lexical blocks and at the end of the function.
+
+extern int use(int);
+int glob1;
+int glob2;
+float globf;
+
+int foo(int i) {
+ // CHECK: define{{.*}}foo
+ if (i < 4) {
+ int j = i * 3;
+ if (glob1 > 3) {
+ float f = globf;
+ // CHECK: [[SSAVAL:%[a-z0-9]*]] = load float{{.*}}globf
+ j = f;
+ glob2 = j;
+ // CHECK: store{{.*}}glob2
+ // CHECK-NEXT: call void (...) @llvm.fake.use(float [[SSAVAL]])
+ }
+ glob1 = j;
+ // CHECK: store{{.*}}glob1
+ // CHECK-NEXT: call void (...) @llvm.fake.use(i32 %j.
+ }
+ // CHECK: call void (...) @llvm.fake.use(i32 %i)
+ // CHECK-NEXT: ret
+ return 4;
+}
diff --git a/clang/test/CodeGen/extend-liveness2.cpp b/clang/test/CodeGen/extend-liveness2.cpp
new file mode 100644
index 00000000000000..119c783c634806
--- /dev/null
+++ b/clang/test/CodeGen/extend-liveness2.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-lifetimes -fcxx-exceptions -fexceptions -o - | FileCheck %s
+// REQUIRES: x86-registered-target
+// This test checks that the fake_use concept works with exception handling and that we
+// can handle the __int128 data type.
+
+class A {
+public:
+ A(int i) : m_i(i) {}
+ void func(__int128 i128);
+
+ int m_i;
+};
+
+extern int bar();
+extern void foo();
+int glob;
+
+void A::func(__int128 i128) {
+ int j = 4;
+ try {
+ int k = bar();
+ foo();
+ // CHECK: [[SSAVAL:%[a-z0-9]*]] = invoke{{.*}}bar
+ glob = 0;
+ // CHECK: store{{.*}}glob
+ // CHECK-NEXT: call void (...) @llvm.fake.use(i32 [[SSAVAL]])
+ } catch (...) {
+ foo();
+ }
+ // CHECK-LABEL: try.cont:
+ // CHECK-DAG: call void (...) @llvm.fake.use({{.*%this}})
+ // CHECK-DAG: call void (...) @llvm.fake.use(i128 %i128.sroa.0.0.insert.insert)
+ // CHECK: ret void
+}
diff --git a/clang/test/CodeGen/fake-use-determinism.c b/clang/test/CodeGen/fake-use-determinism.c
new file mode 100644
index 00000000000000..d62efbb4efe7ee
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-determinism.c
@@ -0,0 +1,18 @@
+// RUN: %clang -S -O2 -emit-llvm -fextend-lifetimes %s -o - | FileCheck %s
+// REQUIRES: asserts
+//
+// We are checking that the fake.use calls for i, j and k appear
+// in a particular order. It is not the order itself that is important
+// but that it remains the same between different test runs.
+
+// CHECK: call {{.*}}void (...) @llvm.fake.use(i32 %k)
+// CHECK-NEXT: call {{.*}}void (...) @llvm.fake.use(i32 %j)
+// CHECK-NEXT: call {{.*}}void (...) @llvm.fake.use(i32 %i)
+
+extern void bar();
+void foo(int i, int j, int k)
+{
+ for (int l = 0; l < i; l++) {
+ bar();
+ }
+}
diff --git a/clang/test/CodeGen/fake-use-lambda.cpp b/clang/test/CodeGen/fake-use-lambda.cpp
new file mode 100644
index 00000000000000..fd4881b9bcd531
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-lambda.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 %s -triple=%itanium_abi_triple -O1 -emit-llvm -fextend-lifetimes -o - | FileCheck %s
+// Make sure we don't crash compiling a lambda that is not nested in a function.
+// We also check that fake uses are properly issued in lambdas.
+
+int glob;
+
+extern int foo();
+
+struct S {
+ static const int a;
+};
+
+const int S::a = [](int b) __attribute__((noinline)) {
+ return b * foo();
+}
+(glob);
+
+int func(int param) {
+ return ([=](int lambdaparm) __attribute__((noinline))->int {
+ int lambdalocal = lambdaparm * 2;
+ return lambdalocal;
+ }(glob));
+}
+
+// We are looking for the first lambda's call operator, which should contain
+// 2 fake uses, one for 'b' and one for its 'this' pointer (in that order).
+// The mangled function name contains a $_0, followed by 'cl'.
+// This lambda is an orphaned lambda, i.e. one without lexical parent.
+//
+// CHECK: define internal {{.+\"_Z.+\$_0.*cl.*\"}}
+// CHECK-NOT: ret
+// CHECK: fake.use(i32
+// CHECK-NOT: ret
+// CHECK: fake.use(ptr
+
+// The second lambda. We are looking for 3 fake uses.
+// CHECK: define internal {{.+\"_Z.+\$_0.*cl.*\"}}
+// CHECK-NOT: ret
+// CHECK: fake.use(i32
+// CHECK-NOT: ret
+// CHECK: fake.use(i32
+// CHECK-NOT: ret
+// CHECK: fake.use(ptr
diff --git a/clang/test/CodeGen/fake-use-landingpad.c b/clang/test/CodeGen/fake-use-landingpad.c
new file mode 100644
index 00000000000000..91526f29936146
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-landingpad.c
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -O3 -emit-llvm -fextend-lifetimes -fexceptions -o - | FileCheck %s
+
+// Check that fake uses do not mistakenly cause a landing pad to be generated when
+// exceptions are enabled.
+
+extern void bar(int);
+void foo(int p) {
+ int a = 17;
+ bar(a);
+}
+
+// CHECK: define {{.*}} @foo
+// CHECK-NOT: personality
+// CHECK: entry:
+// CHECK: llvm.fake.use
+// CHECK-NOT: landingpad
diff --git a/clang/test/CodeGen/fake-use-noreturn.cpp b/clang/test/CodeGen/fake-use-noreturn.cpp
new file mode 100644
index 00000000000000..acfcb0ce90b5e4
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-noreturn.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang %s -S -emit-llvm -fextend-lifetimes -O2 -o - -fno-discard-value-names | FileCheck %s
+//
+// Check we can correctly produce fake uses for function-level variables even
+// when we have a return in a nested conditional and there is no code at the end
+// of the function.
+
+// CHECK-LABEL: define{{.*}}@_Z3fooi
+// CHECK: call{{.*}}llvm.fake.use(i32 %i)
+// CHECK-LABEL: define{{.*}}@_ZN1C3barEi
+// CHECK-DAG: call{{.*}}llvm.fake.use(i32 %i)
+// CHECK-DAG: call{{.*}}llvm.fake.use({{.*}}%this)
+
+void foo(int i) {
+ while (0)
+ if (1)
+ return;
+}
+
+class C {
+ void bar(int i);
+};
+
+void C::bar(int i) {
+ while (0)
+ if (1)
+ return;
+}
diff --git a/clang/test/CodeGen/fake-use-return-line.c b/clang/test/CodeGen/fake-use-return-line.c
new file mode 100644
index 00000000000000..0c4377058d6628
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-return-line.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -emit-llvm -O2 -debug-info-kind=limited -fextend-lifetimes -o - %s | FileCheck %s
+
+// Clang adjusts the line numbers of returns based on the line numbers of
+// dominating stores to %retval; we test that fake use intrinsics do not affect
+// this, and the return is given the correct line.
+
+// CHECK: define{{.*}}@main
+// CHECK: ret i32{{.*}}!dbg ![[MDINDEX:[0-9]*]]
+// CHECK: ![[MDINDEX]] = !DILocation(line: [[# @LINE + 5]]
+int main()
+{
+ volatile int a = 1;
+ int b = a + 2;
+ return b;
+}
diff --git a/clang/test/CodeGen/fake-use-sanitizer.cpp b/clang/test/CodeGen/fake-use-sanitizer.cpp
new file mode 100644
index 00000000000000..915924a2b31025
--- /dev/null
+++ b/clang/test/CodeGen/fake-use-san...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SLTozer
force-pushed
the
implement-extend-lifetimes-flag
branch
from
e04a16c to
7d8646e
Compare
|
Ping |
|
Ping - if it helps reviewers, most of the line changes in this patch are test additions (
Review for any of these areas individually is useful, even if it's only one part of the patch; I can't easily break the patch down further, since this is the minimum set needed to correctly produce llvm.fake.use intrinsics correctly from Clang (along with the prior patch #110000 which adds the flag itself). |
There was a problem hiding this comment.
Behold, I've added a bunch of pedantic comments about the tests.
I think a significant matter is that they all run an LLVM optimisation pipeline, which I believe means they cover too much of the project to be "clang" tests, they're more end-to-end or cross project tests. Potentially we can move them there; it's more preferable to test for the exact output of clang without the optimisation passes instead though.
(If there's optimisation behaviour that does need testing, it should be part of the relevant LLVM pass).
| } | ||
| glob1 = j; | ||
| // CHECK: store{{.*}}glob1 | ||
| // CHECK-NEXT: call void (...) @llvm.fake.use(i32 %j. |
There was a problem hiding this comment.
Won't %j need capturing in a variable -- IIRC on Release builds we discard the names? Also fixable with -fno-discard-value-names I suppose.
There was a problem hiding this comment.
AFAICT, for tests that invoke %clang_cc1, -fno-discard-value-names is the default - for value names to be discarded, the driver needs to pass -discard-value-names to -cc1.
| @@ -0,0 +1,34 @@ | |||
| // RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-lifetimes -fcxx-exceptions -fexceptions -o - | FileCheck %s | |||
| // REQUIRES: x86-registered-target | |||
There was a problem hiding this comment.
Requires x86 as a target, but doesn't specify a triple?
| // This test checks that the fake_use concept works with exception handling and that we | ||
| // can handle the __int128 data type. |
There was a problem hiding this comment.
IMO this can be more direct: "Test that fake uses of values defined in exception blocks..." etc. "... and that we emit fake uses of int128s". The future reader adding some subtle semantic shift is going to want to know the exact behaviour demanded.
| @@ -0,0 +1,18 @@ | |||
| // RUN: %clang -S -O2 -emit-llvm -fextend-lifetimes %s -o - | FileCheck %s | |||
| // REQUIRES: asserts | |||
There was a problem hiding this comment.
Is this REQUIRES a proxy for needing value-names, as above?
| // The mangled function name contains a $_0, followed by 'cl'. | ||
| // This lambda is an orphaned lambda, i.e. one without lexical parent. | ||
| // | ||
| // CHECK: define internal {{.+\"_Z.+\$_0.*cl.*\"}} |
There was a problem hiding this comment.
CHECK-LABEL, and for the other define below?
clang/test/CodeGen/fake-use-scalar.c
Outdated
| struct A s; | ||
| volatile int vloc; | ||
| struct A v[128]; | ||
| char c[33]; | ||
| return 0; |
There was a problem hiding this comment.
Should we not be initializing these items -- for the volatile at least, there's no defined value for it at any point, so it'd hard to generate a fake use anyway.
There was a problem hiding this comment.
We don't need initialization to generate fake uses - adding some positive tests should give assurance within the test itself that initializing is unnecessary.
| @@ -0,0 +1,24 @@ | |||
| // RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-lifetimes -disable-llvm-passes -o - | FileCheck %s | |||
| // Check that we generate a fake_use call for small aggregate types. | |||
There was a problem hiding this comment.
So, aggregate types that get splatted into single integers? It's worth identifying that in fake-use-scalar.c just in case someone tries to add test coverage there IMHO.
| extern struct s inits(); | ||
| void foo(struct s S) | ||
| { | ||
| int arr[4]; |
There was a problem hiding this comment.
What sets this apart as an aggregate type to be fake-used, and the character array in fake-use-scalar.c?
| // CHECK-DAG: %[[FAKEUSE1:[^ ]+]] = load{{.*}} %loc, | ||
| // CHECK-DAG: call{{.*}}llvm.fake.use({{.*}}%[[FAKEUSE1]]) | ||
| // CHECK-DAG: %[[FAKEUSE2:[^ ]+]] = load{{.*}} %arr, | ||
| // CHECK-DAG: call{{.*}}llvm.fake.use({{.*}}%[[FAKEUSE2]]) | ||
| // CHECK-DAG: %[[FAKEUSE3:[^ ]+]] = load{{.*}} %S, | ||
| // CHECK-DAG: call{{.*}}llvm.fake.use({{.*}}%[[FAKEUSE3]]) |
There was a problem hiding this comment.
Better to remove the -DAG part and explicitly test that the fake uses are at the end of the function?
clang/test/CodeGen/fake-use.cpp
Outdated
| // RUN: %clang_cc1 %s -O2 -emit-llvm -fextend-this-ptr -o - | FileCheck %s | ||
| // Check that we generate a fake_use call with the 'this' pointer as argument. | ||
| // The call should appear after the call to bar(). |
There was a problem hiding this comment.
Hard to see why the rest of the class information in this test is necessary? There's a whole ton of fake uses generated that aren't tested for.
IMO: this is a good test for some other things, like which of the variables get fake uses (not params but yes locals?) and how the lifetime of the locally-allocated object 'V' is handled: seemingly not because it's returned, but what would happen if it wasn't?
| // Check we can correctly produce fake uses for function-level variables even | ||
| // when we have a return in a nested conditional and there is no code at the end | ||
| // of the function. |
There was a problem hiding this comment.
I see this is now O0 and disable-O0-optnone, would this not be even better spelt as just -disable-llvm-passes? IMHO that's a much more substantially direct way of signalling "none of LLVM will run".
| // This test verifies that UBSan does not produce null-checks for arguments to | ||
| // llvm.fake.use, and that fake uses are not emitted for a variable on paths | ||
| // it has not been declared. |
There was a problem hiding this comment.
Thanks for regenerating this with no optimisations happening; am I right in thinking that -fsanitize=null doesn't just turn off the sanitizer, it turns on ubsan for null values or something? I see there are calls to ubsantrap being generated, and want to settle my understanding of this.
There was a problem hiding this comment.
Correct, it turns on sanitizer checks for null values - I was confused at first, especially since the comment previously referenced the Address Sanitizer. In short, we're simply testing that we don't try to sanitize fake uses, for obvious reasons.
| // RUN: %clang_cc1 %s -O0 -emit-llvm -fextend-this-ptr -o - | FileCheck %s \ | ||
| // RUN: --implicit-check-not="call void (...) @llvm.fake.use" | ||
| // RUN: %clang_cc1 %s -O0 -emit-llvm -fextend-lifetimes -o - | FileCheck %s \ | ||
| // RUN: --implicit-check-not="call void (...) @llvm.fake.use" | ||
| // RUN: %clang_cc1 %s -O1 -emit-llvm -fextend-this-ptr -o - | FileCheck -check-prefix=OPT %s | ||
| // RUN: %clang_cc1 %s -O1 -emit-llvm -fextend-lifetimes -o - | FileCheck -check-prefix=OPT %s | ||
| // RUN: %clang_cc1 %s -Os -emit-llvm -fextend-this-ptr -o - | FileCheck -check-prefix=OPT %s | ||
| // RUN: %clang_cc1 %s -Os -emit-llvm -fextend-lifetimes -o - | FileCheck -check-prefix=OPT %s | ||
| // RUN: %clang_cc1 %s -Oz -emit-llvm -fextend-this-ptr -o - | FileCheck -check-prefix=OPT %s | ||
| // RUN: %clang_cc1 %s -Oz -emit-llvm -fextend-lifetimes -o - | FileCheck -check-prefix=OPT %s | ||
| // Check that we do not generate a fake_use call when we are not optimizing. |
There was a problem hiding this comment.
I reckon these can all be given --disable-llvm-passes too, as what we're checking is the effect of the optimisation flag on clang, not on the actual pipeline.
clang/lib/CodeGen/CGCleanup.cpp
Outdated
| EHScopeStack::stable_iterator Old) const { | ||
| for (EHScopeStack::iterator it = begin(); stabilize(it) != Old; it++) { | ||
| EHCleanupScope *cleanup = dyn_cast<EHCleanupScope>(&*it); | ||
| if (!cleanup || !cleanup->isLifetimeMarker()) | ||
| if (!cleanup || !(cleanup->isLifetimeMarker() || cleanup->isFakeUse())) |
There was a problem hiding this comment.
I feel like the number of inversions here makes it harder to read; how about the suggestion,
| if (!cleanup || !(cleanup->isLifetimeMarker() || cleanup->isFakeUse())) | |
| if (!cleanup) | |
| return false | |
| if (!cleanup->isLifetimeMarker() && !cleanup->isFakeUse())) | |
| return false; |
I.e. it's slightly clearer that it's a dont-dereference-null check and a is-it-not-a-noop check.
clang/lib/CodeGen/CGDecl.cpp
Outdated
| // We do not emit a fake use if we want to apply optnone to this function, | ||
| // even if we might not apply it anyway due to minsize or similar attributes. | ||
| if (!CGM.getCodeGenOpts().DisableO0ImplyOptNone && | ||
| CGM.getCodeGenOpts().OptimizationLevel == 0) | ||
| return; |
There was a problem hiding this comment.
Hmmmmm. It feels a bit wrong to bake this kind of high level decision this deep into the code. Would it be better to instead have the clang driver decide whether to enable fextend-lifetimes depending on the optimisation mode, then pass the flag to cc1, and not have this kind of check?
This would reduce our testing matrix in the future to separate "cc1 does fextend lifetimes correctly" from "when should fextend-lifetimes be done".
There was a problem hiding this comment.
Yes - that's how it originally worked, and the choice being moved to here is an accidental hangover from some earlier attempts at modifications I made; specifically I tried to make it so that fake uses would only be applied if the specific function did not have optnone, but the decision-making that applies optnone to functions happens after we finalize fake uses. I'm currently considering whether it's a good idea to just enable -fextend-lifetimes at O0, in which case this behaviour can be removed outright, otherwise it could be moved back to the driver as you've suggested.
There was a problem hiding this comment.
There's no value to enabling -fextend-lifetimes at O0, because we only emit stack-home locations at O0, which are valid for the entire function. However, I don't remember whether that decision is a blanket decision based on opt level or per-function based on optnone. If it's based on opt level, then adding fake-uses to an optnone function (at O1 and up) might actually be the right thing to do.
There was a problem hiding this comment.
(FWIW - clang IRGen always emit stack-home locations, at any optimization level - it's only a question of whether we run the optimizations (& whether they fire, eg: we might run the optimization pass, but it detects a function is optnone, then it does nothing) that remove those stack homes)
There was a problem hiding this comment.
The thoughts I have are that by default, in a typical -O0 build, -fextend-lifetimes is useless. But there may be atypical circumstances that would make it useful - such as following the somewhat common pattern (in LLVM development) of compiling with -O0 -emit-llvm -disable-O0-optnone to produce unoptimized IR (except for compulsory optimizations) from clang to pass to opt or some other tool that may modify it; in these cases if a user has explicitly requested -fextend-lifetimes, it seems reasonable to me to still emit fake uses. The argument against emitting fake uses is that they have no value, but given that the flag is opt-in only I think it's fine to trust that the user isn't going to accidentally add a flag that they don't need.
Also, I just realized that my comment above had some bad wording - by "enable extend lifetimes at O0", I meant to not automatically disable the flag's effects at O0, not to turn it on by default at O0.
There was a problem hiding this comment.
Objection withdrawn. Because it's opt-in, we should do what the user requested. As you say, in a normal -O0 situation all this does is increase IR size for no benefit, but it has its uses.
There was a problem hiding this comment.
(might be worth removing "-disable-O0-optnone" as it's a bit of a hazard for reasons like this - if folks want optimizable IR, but without optimizations applied, they'd probably be better off disabling optimizations (-disable-llvm-optzns) I think?)
clang/lib/CodeGen/CGDecl.cpp
Outdated
|
|
||
| // Helper function to determine whether a variable's or parameter's lifetime | ||
| // should be extended. | ||
| static bool extendLifetime(const ASTContext &Context, const Decl *FuncDecl, |
There was a problem hiding this comment.
IMO: this function should be named shouldExtendLifetime
clang/lib/CodeGen/CGDecl.cpp
Outdated
| @@ -1664,6 +1710,17 @@ CodeGenFunction::EmitAutoVarAlloca(const VarDecl &D) { | |||
| emission.getOriginalAllocatedAddress(), | |||
| emission.getSizeForLifetimeMarkers()); | |||
|
|
|||
| // Analogous to lifetime markers, we use a 'cleanup' to emit fake.use | |||
| // calls for local variables. We are exempting volatile variables and | |||
| // non-scalars larger than 4 times the size of an unsigned int (32 bytes). | |||
There was a problem hiding this comment.
16 bytes presumably, or 32 bits for unsigned int? Best to avoid being too specific IMO as it's machine dependent. (Perhaps all the world's a VAX, but we can avoid saying it in comments).
| if (CGM.getCodeGenOpts().ExtendLifetimes) { | ||
| if (extendLifetime(getContext(), CurCodeDecl, D, CXXABIThisDecl)) | ||
| EHStack.pushCleanup<FakeUse>(NormalFakeUse, | ||
| emission.getAllocatedAddress()); |
There was a problem hiding this comment.
I see the nearby EHStack cleanup code uses getOriginalAllocatedAddress -- should we be using that instead? Related comments talk about there being a difference the address and the object; dunno what to make of that, but it's worth checking.
There was a problem hiding this comment.
I'll look into it - I'm not familiar enough to know, and a quick test indicates it's nothing obviously wrong, but I'll try to verify if it should change or not.
There was a problem hiding this comment.
Looks like the difference is about allocas in a non-default address space - essentially we'll get either:
%i = alloca i32, align 4, addrspace(5)
%i.ascast = addrspacecast ptr addrspace(5) %i to ptr
store i32 %someval, ptr %i.ascast, align 4
; getAllocatedAddress
%fake.use = load i32, ptr %i.ascast, align 4
call void (...) @llvm.fake.use(i32 %fake.use)
; getOriginalAllocatedAddress
%fake.use = load i32, ptr addrspace(5) %i, align 4
call void (...) @llvm.fake.use(i32 %fake.use)
In practice, both of them result in extended variable lifetimes (the fake use will continue to track %someval in both cases), so I think this is unimportant. On the one hand, the pointer that we use to store and load is %i.ascast, so it seems consistent for the fake use to use that; on the other hand, if a #dbg_declare is created, it will refer to ptr addrspace(5) %i, so that could be more appropriate for the fake use. In summary: I don't know, and I don't think it matters. I slightly lean towards the getAllocatedAddress version.
Following the previous patch which adds the "extend lifetimes" flag without (almost) any functionality, this patch adds the real feature by allowing Clang to emit fake uses. These are emitted as a new form of cleanup, set for variable addresses, which just emits a fake use intrinsic when the variable falls out of scope. The code for achieving this is simple, with most of the logic centered on determining whether to emit a fake use for a given address, and on ensuring that fake uses are ignored in a few cases.
…ow fake use emission with disable-O0-optnone
SLTozer
force-pushed
the
implement-extend-lifetimes-flag
branch
from
7ce4159 to
124ada4
Compare
|
I've removed the logic that disables this flag at O0 - this has also removed the need for |
|
Post-holiday ping; I think all the outstanding comments have been addressed, with some minor changes since the last round of review - primarily referencing the new CodeGenOpts settings, and removing the O0-related logic. |
|
Ping |
| if (II->getIntrinsicID() == llvm::Intrinsic::lifetime_end) | ||
| continue; | ||
|
|
||
| if (II->getIntrinsicID() == llvm::Intrinsic::fake_use) { | ||
| LoadIntoFakeUse = dyn_cast<llvm::Instruction>(II->getArgOperand(0)); |
There was a problem hiding this comment.
This could be nitpicking now; but we don't test that it's actually a load, right? So technically a legitimate instruction that's followed by a fake use might be ignored? (Perhaps this is a situation that never occurs).
There was a problem hiding this comment.
It's looking at the operand, not the preceding instruction. CodeGenFunction::EmitFakeUse generates the operand as a load, and that's the only place fake uses get created IIUC. You could have an assert here that the operand is a load.
There was a problem hiding this comment.
All fake uses at this stage have a load (which should be the immediately preceding instruction) as their operand, since fake uses are implemented as cleanups for stack variables. A check that it's a load could be added here, but if anything I'd say an assert would be preferred, since if the above fact ever changes this will need to be revisited.
Edit: Beaten to the punch! But it seems there's agreement that an assertion makes sense.
clang/lib/CodeGen/CGCleanup.h
Outdated
| @@ -87,6 +87,10 @@ class EHScope { | |||
| LLVM_PREFERRED_TYPE(bool) | |||
| unsigned IsLifetimeMarker : 1; | |||
|
|
|||
| /// Whether this cleanup is a fake use | |||
| if (II->getIntrinsicID() == llvm::Intrinsic::lifetime_end) | ||
| continue; | ||
|
|
||
| if (II->getIntrinsicID() == llvm::Intrinsic::fake_use) { | ||
| LoadIntoFakeUse = dyn_cast<llvm::Instruction>(II->getArgOperand(0)); |
There was a problem hiding this comment.
It's looking at the operand, not the preceding instruction. CodeGenFunction::EmitFakeUse generates the operand as a load, and that's the only place fake uses get created IIUC. You could have an assert here that the operand is a load.
Following the previous patch which adds the "extend lifetimes" flag without (almost) any functionality, this patch adds the real feature by allowing Clang to emit fake uses. These are emitted as a new form of cleanup, set for variable addresses, which just emits a fake use intrinsic when the variable falls out of scope. The code for achieving this is simple, with most of the logic centered on determining whether to emit a fake use for a given address, and on ensuring that fake uses are ignored in a few cases. Co-authored-by: Stephen Tozer <[email protected]>
|
As with the previous commit #110000, this has been merged in 4424c44c, via command line so as to provide proper commit author attribution to @wolfy1961 (as this doesn't seem to be possible via the web interface). I'm currently looking at a couple buildbot errors; one build failure on several sanitizer bots due to using a deprecated function (trivial update should fix), one test failure on a clang-armv8 bot that at-a-glance looks like the test might be assuming things about the output that aren't true for all arches. |
Fixes two buildbot errors caused by 4424c44 (#110102): The first error, seen on some sanitizer bots: https://lab.llvm.org/buildbot/#/builders/51/builds/9901 The initial commit used the deprecated getDeclaration intrinsic instead of the non-deprecated getOrInsert- equivalent. This patch trivially updates the code in question to use the new intrinsic. The second error, seen on the clang-armv8-quick bot: https://lab.llvm.org/buildbot/#/builders/154/builds/10983 One of the tests depends on a particular triple to get the exact output expected by the test, but did not specify this triple; this patch adds the triple in question.
|
Pushed up a fix-forward commit, 8ad9e1e, that should fix the above issues; if issues still remain, I'll revert and recommit after fixing offline. |
|
This recently added tests failed on Windows like |
|
It's a name mangling issue on Windows (or maybe other OS). For example, the check However, the compiler on Windows outputs |
|
I think the forward fix has been identified, and importantly it will take a bit longer to revert than to submit the fix (adding triples to the tests), because there are 4 commits that need to be reverted and I need to run a quick build to ensure that nothing is broken by the revert itself - I'm running that build right now, but will push the fix commit up while that build is running since that might fix the problem sooner. |
Attempted fix for errors observed on: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8724497761651154417/overview Several tests that required an explicit triple had none present; this patch adds those triples.
Commented on the commit but I'm not sure if that sends notifications. FWIW I think you can use |
I saw it, thanks - I'll likely update the tests afterwards, though for now I'm keeping an eye on the bots/merge to make sure things turn/stay green. |
Attempted fix for errors observed on: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8724466517233166081/overview Following the previous fixes, one test that needed an explicit triple was missed; this commit adds that triple.
|
I made a mistake and missed one of the tests in the fix-forward commit - the build with the fix commit only recently finished, so I've pushed up a second fix now - since the other tests were fixed by this, I expect the last one should be as well. I can't continue to monitor this review right now, and there are now a number of small patches stacked on top of this one - in case there's some further error that requires a revert, the commit here is a clean and (locally) green revert of this and all dependent patches, to save other users the trouble of manually unpicking them. |
Thanks for the fix, and I verified that it fixes the test failures on Windows. |
llvm/llvm-project#110102 added new tests that are broken in Windows. This patch disables those tests on Windows as a workaround to fix the toolchain builders. Original-Bug: 392954079 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/1193839 Original-Revision: 45e0deaa8ba8298493595c2fb6a01432f5b2938d GitOrigin-RevId: b9d00d6bc0b342c12f45324ae9df990ead7f8853 Change-Id: I9638e767e6adf5cbcc487ac32e9cef0478818a51
Following the previous patch which adds the "extend lifetimes" flag without (almost) any functionality, this patch adds the real feature by allowing Clang to emit fake uses. These are emitted as a new form of cleanup, set for variable addresses, which just emits a fake use intrinsic when the variable falls out of scope. The code for achieving this is simple, with most of the logic centered on determining whether to emit a fake use for a given address, and on ensuring that fake uses are ignored in a few cases.
All code originally written by @wolfy1961, while I'll be handling the review.