[clang-tools-extra] [llvm] [clang] Dont alter cold function alignment unless using Os (PR #72387)
https://github.com/FlameTop created
https://github.com/llvm/llvm-project/pull/72387
This PR alters the behaviour of function alignment for functions marked as
'cold'. Currently functions marked with a ‘cold’ attribute are also set as
optimize by size. Optimize by size alters the function alignment from default.
This interferes with code replacement features on our targets. This PR allows
cold functions to maintain their default alignment except when optimize by size
is explicitly used.
>From 49dd8a3a61ce29e71c3dcc81358c260dc2e37912 Mon Sep 17 00:00:00 2001
From: Phil Camp
Date: Wed, 15 Nov 2023 13:12:33 +
Subject: [PATCH] Dont alter cold function alignment unless Os used
---
clang/lib/CodeGen/CodeGenModule.cpp | 3 ++
clang/test/CodeGen/cold-align.cpp| 32 +
llvm/include/llvm/IR/Attributes.td | 1 +
llvm/lib/CodeGen/MachineFunction.cpp | 3 +-
llvm/test/CodeGen/X86/cold-align.ll | 52
5 files changed, 90 insertions(+), 1 deletion(-)
create mode 100644 clang/test/CodeGen/cold-align.cpp
create mode 100644 llvm/test/CodeGen/X86/cold-align.ll
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp
b/clang/lib/CodeGen/CodeGenModule.cpp
index 75355282878f2f3..447748bf7fa90fb 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2409,6 +2409,9 @@ void
CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
if (!ShouldAddOptNone)
B.addAttribute(llvm::Attribute::OptimizeForSize);
B.addAttribute(llvm::Attribute::Cold);
+ // dont alter alignment if not optimizing for size
+ if (!CodeGenOpts.OptimizeSize)
+B.addAttribute("keepalign", "true");
}
if (D->hasAttr())
B.addAttribute(llvm::Attribute::Hot);
diff --git a/clang/test/CodeGen/cold-align.cpp
b/clang/test/CodeGen/cold-align.cpp
new file mode 100644
index 000..93564d181d65d96
--- /dev/null
+++ b/clang/test/CodeGen/cold-align.cpp
@@ -0,0 +1,32 @@
+// Dont alter function alignment if marked cold
+//
+// Cold attribute marks functions as also optimize for size. This normally
collapses the
+// default function alignment. This can interfere with edit&continue
effectiveness.
+//
+// RUN: %clang -O2 -S -emit-llvm %s -o - | FileCheck %s -check-prefixes TWO
+// RUN: %clang -Os -S -emit-llvm %s -o - | FileCheck %s -check-prefixes
SIZE
+
+class Dismissed
+{
+public:
+ __attribute__((cold)) void Chilly();
+void Temparate();
+ __attribute__((hot)) void Sizzle();
+};
+void Dismissed::Chilly(){};
+void Dismissed::Temparate(){};
+void Dismissed::Sizzle(){};
+
+// TWO: attributes #0 = {
+// TWO: "keepalign"="true"
+// TWO: attributes #1 = {
+// TWO-NOT: "keepalign"="true"
+// TWO: attributes #2 = {
+// TWO-NOT: "keepalign"="true"
+
+// SIZE: attributes #0 = {
+// SIZE-NOT: "keepalign"="true"
+// SIZE: attributes #1 = {
+// SIZE-NOT: "keepalign"="true"
+// SIZE: attributes #2 = {
+// SIZE-NOT: "keepalign"="true"
\ No newline at end of file
diff --git a/llvm/include/llvm/IR/Attributes.td
b/llvm/include/llvm/IR/Attributes.td
index fc38e68ad273b6b..30844f566786c37 100644
--- a/llvm/include/llvm/IR/Attributes.td
+++ b/llvm/include/llvm/IR/Attributes.td
@@ -332,6 +332,7 @@ def NoJumpTables : StrBoolAttr<"no-jump-tables">;
def NoInlineLineTables : StrBoolAttr<"no-inline-line-tables">;
def ProfileSampleAccurate : StrBoolAttr<"profile-sample-accurate">;
def UseSampleProfile : StrBoolAttr<"use-sample-profile">;
+def KeepAlign: StrBoolAttr<"keepalign">;
def DenormalFPMath : ComplexStrAttr<"denormal-fp-math", [FnAttr]>;
def DenormalFPMathF32 : ComplexStrAttr<"denormal-fp-math-f32", [FnAttr]>;
diff --git a/llvm/lib/CodeGen/MachineFunction.cpp
b/llvm/lib/CodeGen/MachineFunction.cpp
index 07eb0ba7f45c2e3..ab5392a05fc019b 100644
--- a/llvm/lib/CodeGen/MachineFunction.cpp
+++ b/llvm/lib/CodeGen/MachineFunction.cpp
@@ -214,7 +214,8 @@ void MachineFunction::init() {
// FIXME: Shouldn't use pref alignment if explicit alignment is set on F.
// FIXME: Use Function::hasOptSize().
- if (!F.hasFnAttribute(Attribute::OptimizeForSize))
+ if ((!F.hasFnAttribute(Attribute::OptimizeForSize)) ||
+ (F.getFnAttribute("keepalign").getValueAsBool()))
Alignment = std::max(Alignment,
STI->getTargetLowering()->getPrefFunctionAlignment());
diff --git a/llvm/test/CodeGen/X86/cold-align.ll
b/llvm/test/CodeGen/X86/cold-align.ll
new file mode 100644
index 000..ab1f393dfa561ab
--- /dev/null
+++ b/llvm/test/CodeGen/X86/cold-align.ll
@@ -0,0 +1,52 @@
+; Dont alter function alignment if marked cold
+;
+; Cold attribute marks functions as also optimize for size. This normally
collapses the
+; default function alignment. This can interfere with edit&continue
effectiveness.
+;
+;
+; RUN: llc -O2 <%s | FileCheck %s -check-prefixes TWO
+;
+; TWO: .globl _ZN9Dismissed6ChillyEv
+; TWO-NEXT: .p2align 4, 0x90
+; TWO: .globl _ZN9
[clang] [clang-tools-extra] [llvm] Dont alter cold function alignment unless using Os (PR #72387)
FlameTop wrote: @pogo59 I agree that stating the PS ABI has an explicit function alignment unless overridden does negate the need for a new attribute. I will re-work the code to remove the new attribute and instead enforce it for the PS ABI target. https://github.com/llvm/llvm-project/pull/72387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27043: Remove assertion on analysis of rvalue vector
FlameTop created this revision.
FlameTop added reviewers: zaks.anna, dcoughlin.
FlameTop added a subscriber: cfe-commits.
ExprEngine::VisitLvalArraySubscriptExpr contains an assert to ensure it is
passed either a GLvalue or a small number of C exemptions. This change adds
vector types to the assert as these also can be rvalues.
A provided test ensures the assertion no longer fires for vector rvalues.
https://reviews.llvm.org/D27043
Files:
llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
llvm/tools/clang/test/Analysis/crash-vector-rvalue.c
Index: llvm/tools/clang/test/Analysis/crash-vector-rvalue.c
===
--- /dev/null
+++ llvm/tools/clang/test/Analysis/crash-vector-rvalue.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+typedef int vec __attribute__((__vector_size__(4)));;
+vec bar();
+int foo() {
+ int op = bar()[2];
+ return op;
+}
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1973,7 +1973,8 @@
StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
assert(A->isGLValue() ||
- (!AMgr.getLangOpts().CPlusPlus &&
+ A->getBase()->getType()->isVectorType() ||
+ (!AMgr.getLangOpts().CPlusPlus &&
A->getType().isCForbiddenLValueType()));
for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
Index: llvm/tools/clang/test/Analysis/crash-vector-rvalue.c
===
--- /dev/null
+++ llvm/tools/clang/test/Analysis/crash-vector-rvalue.c
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
+typedef int vec __attribute__((__vector_size__(4)));;
+vec bar();
+int foo() {
+ int op = bar()[2];
+ return op;
+}
Index: llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1973,7 +1973,8 @@
StmtNodeBuilder Bldr(checkerPreStmt, Dst, *currBldrCtx);
assert(A->isGLValue() ||
- (!AMgr.getLangOpts().CPlusPlus &&
+ A->getBase()->getType()->isVectorType() ||
+ (!AMgr.getLangOpts().CPlusPlus &&
A->getType().isCForbiddenLValueType()));
for (ExplodedNodeSet::iterator it = checkerPreStmt.begin(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D19866: [Analyzer] Correct stack address escape diagnostic
FlameTop created this revision.
FlameTop added reviewers: zaks.anna, dcoughlin.
FlameTop added a subscriber: cfe-commits.
Leaking a stack address via a static variable refers to it in the diagnostic as
a 'global'. This patch corrects the diagnostic for static variables.
Patch by Phil Camp, SN Systems
http://reviews.llvm.org/D19866
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
llvm/tools/clang/test/Analysis/stackaddrleak.c
Index: llvm/tools/clang/test/Analysis/stackaddrleak.c
===
--- llvm/tools/clang/test/Analysis/stackaddrleak.c
+++ llvm/tools/clang/test/Analysis/stackaddrleak.c
@@ -19,7 +19,7 @@
p = (const char *) __builtin_alloca(12);
} // expected-warning{{Address of stack memory allocated by call to alloca()
on line 19 is still referred to by the global variable 'p' upon returning to
the caller. This will be a dangling reference}}
-// PR 7383 - previosly the stack address checker would crash on this example
+// PR 7383 - previously the stack address checker would crash on this example
// because it would attempt to do a direct load from 'pr7383_list'.
static int pr7383(__const char *__)
{
@@ -33,7 +33,7 @@
int x;
a = &x;
b = &x;
-} // expected-warning{{Address of stack memory associated with local variable
'x' is still referred to by the global variable 'a' upon returning}}
expected-warning{{Address of stack memory associated with local variable 'x' is
still referred to by the global variable 'b' upon returning}}
+} // expected-warning{{Address of stack memory associated with local variable
'x' is still referred to by the static variable 'a' upon returning}}
expected-warning{{Address of stack memory associated with local variable 'x' is
still referred to by the static variable 'b' upon returning}}
intptr_t returnAsNonLoc() {
int x;
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -226,17 +226,22 @@
if (!BT_stackleak)
BT_stackleak.reset(
-new BuiltinBug(this, "Stack address stored into global variable",
- "Stack address was saved into a global variable. "
+new BuiltinBug(this, "Stack address stored into global/static
variable",
+ "Stack address was saved into a global/static variable.
"
"This is dangerous because the address will become "
"invalid after returning from the function"));
for (unsigned i = 0, e = cb.V.size(); i != e; ++i) {
// Generate a report for this bug.
SmallString<512> buf;
llvm::raw_svector_ostream os(buf);
SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext());
-os << " is still referred to by the global variable '";
+os << " is still referred to by the ";
+if (isa(cb.V[i].first->getMemorySpace()))
+ os << "static";
+else
+ os << "global";
+os << " variable '";
const VarRegion *VR = cast(cb.V[i].first->getBaseRegion());
os << *VR->getDecl()
<< "' upon returning to the caller. This will be a dangling reference";
Index: llvm/tools/clang/test/Analysis/stackaddrleak.c
===
--- llvm/tools/clang/test/Analysis/stackaddrleak.c
+++ llvm/tools/clang/test/Analysis/stackaddrleak.c
@@ -19,7 +19,7 @@
p = (const char *) __builtin_alloca(12);
} // expected-warning{{Address of stack memory allocated by call to alloca() on line 19 is still referred to by the global variable 'p' upon returning to the caller. This will be a dangling reference}}
-// PR 7383 - previosly the stack address checker would crash on this example
+// PR 7383 - previously the stack address checker would crash on this example
// because it would attempt to do a direct load from 'pr7383_list'.
static int pr7383(__const char *__)
{
@@ -33,7 +33,7 @@
int x;
a = &x;
b = &x;
-} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'b' upon returning}}
+} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'b' upon returning}}
intptr_t returnAsNonLoc() {
int x;
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- llvm/tools/clang/lib/Stati
Re: [PATCH] D19866: [Analyzer] Correct stack address escape diagnostic
FlameTop updated this revision to Diff 56591.
FlameTop added a comment.
Thank you for your review. I have removed the changes to the bug description.
Apologies, I did not realize it would alter the hash.
http://reviews.llvm.org/D19866
Files:
llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
llvm/tools/clang/test/Analysis/stackaddrleak.c
Index: llvm/tools/clang/test/Analysis/stackaddrleak.c
===
--- llvm/tools/clang/test/Analysis/stackaddrleak.c
+++ llvm/tools/clang/test/Analysis/stackaddrleak.c
@@ -19,7 +19,7 @@
p = (const char *) __builtin_alloca(12);
} // expected-warning{{Address of stack memory allocated by call to alloca()
on line 19 is still referred to by the global variable 'p' upon returning to
the caller. This will be a dangling reference}}
-// PR 7383 - previosly the stack address checker would crash on this example
+// PR 7383 - previously the stack address checker would crash on this example
// because it would attempt to do a direct load from 'pr7383_list'.
static int pr7383(__const char *__)
{
@@ -33,7 +33,7 @@
int x;
a = &x;
b = &x;
-} // expected-warning{{Address of stack memory associated with local variable
'x' is still referred to by the global variable 'a' upon returning}}
expected-warning{{Address of stack memory associated with local variable 'x' is
still referred to by the global variable 'b' upon returning}}
+} // expected-warning{{Address of stack memory associated with local variable
'x' is still referred to by the static variable 'a' upon returning}}
expected-warning{{Address of stack memory associated with local variable 'x' is
still referred to by the static variable 'b' upon returning}}
intptr_t returnAsNonLoc() {
int x;
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -236,7 +236,12 @@
SmallString<512> buf;
llvm::raw_svector_ostream os(buf);
SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext());
-os << " is still referred to by the global variable '";
+os << " is still referred to by the ";
+if (isa(cb.V[i].first->getMemorySpace()))
+ os << "static";
+else
+ os << "global";
+os << " variable '";
const VarRegion *VR = cast(cb.V[i].first->getBaseRegion());
os << *VR->getDecl()
<< "' upon returning to the caller. This will be a dangling reference";
Index: llvm/tools/clang/test/Analysis/stackaddrleak.c
===
--- llvm/tools/clang/test/Analysis/stackaddrleak.c
+++ llvm/tools/clang/test/Analysis/stackaddrleak.c
@@ -19,7 +19,7 @@
p = (const char *) __builtin_alloca(12);
} // expected-warning{{Address of stack memory allocated by call to alloca() on line 19 is still referred to by the global variable 'p' upon returning to the caller. This will be a dangling reference}}
-// PR 7383 - previosly the stack address checker would crash on this example
+// PR 7383 - previously the stack address checker would crash on this example
// because it would attempt to do a direct load from 'pr7383_list'.
static int pr7383(__const char *__)
{
@@ -33,7 +33,7 @@
int x;
a = &x;
b = &x;
-} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the global variable 'b' upon returning}}
+} // expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'a' upon returning}} expected-warning{{Address of stack memory associated with local variable 'x' is still referred to by the static variable 'b' upon returning}}
intptr_t returnAsNonLoc() {
int x;
Index: llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ llvm/tools/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -236,7 +236,12 @@
SmallString<512> buf;
llvm::raw_svector_ostream os(buf);
SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext());
-os << " is still referred to by the global variable '";
+os << " is still referred to by the ";
+if (isa(cb.V[i].first->getMemorySpace()))
+ os << "static";
+else
+ os << "global";
+os << " variable '";
const VarRegion *VR = cast(cb.V[i].first->getBaseRegion());
os << *VR->getDecl()
<< "' upon returning to the caller. This will be a dangling reference";
Re: [PATCH] D19866: [Analyzer] Correct stack address escape diagnostic
FlameTop added a comment. Thank you for the acceptance. May I please ask you to commit the change for me as I do not have commit access? cheers Phil Camp http://reviews.llvm.org/D19866 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Dont alter cold function alignment unless using Os (PR #72387)
https://github.com/FlameTop closed https://github.com/llvm/llvm-project/pull/72387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Dont alter cold function alignment unless using Os (PR #72387)
FlameTop wrote: Closing this PR. I will redesign the change and re-submit. https://github.com/llvm/llvm-project/pull/72387 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
