[LLVM] Refactor Autoupgrade function attributes from Module attributes. #84494

DanielKristofKiss · 2024-03-08T14:45:54Z

Refactoring #82763. Caching the module attributes should help with the performance.
Patch also corrects the b-key handling.

llvmbot · 2024-03-08T14:46:24Z

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-elf
@llvm/pr-subscribers-lto

@llvm/pr-subscribers-llvm-ir

Author: Dani (DanielKristofKiss)

Changes

Refactoring #82763. Caching the module attributes should help.

Full diff: https://github.com/llvm/llvm-project/pull/84494.diff

1 Files Affected:

(modified) llvm/lib/IR/AutoUpgrade.cpp (+24-47)

diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 25992395e471b3..4c351a69fabf8e 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -5185,62 +5185,39 @@ static bool isModuleAttributeSet(Module &M, const StringRef &ModAttr) {
   return Attr && Attr->getZExtValue();
 }
 
-// Copy an attribute from module to the function if exists.
-// First value of the pair is used when the module attribute is not zero
-// the second otherwise.
-static void
-CopyModuleAttributeToFunction(Function &F, StringRef FnAttrName,
-                              StringRef ModAttrName,
-                              std::pair<StringRef, StringRef> Values) {
-  if (F.hasFnAttribute(FnAttrName))
-    return;
-  F.addFnAttr(FnAttrName, isModuleAttributeSet(*F.getParent(), ModAttrName)
-                              ? Values.first
-                              : Values.second);
-}
-
-// Copy a boolean attribute from module to the function if exists.
-// Module attribute treated false if zero otherwise true.
-static void CopyModuleAttributeToFunction(Function &F, StringRef AttrName) {
-  CopyModuleAttributeToFunction(
-      F, AttrName, AttrName,
-      std::make_pair<StringRef, StringRef>("true", "false"));
-}
-
-// Copy an attribute from module to the function if exists.
-// First value of the pair is used when the module attribute is not zero
-// the second otherwise.
-static void
-CopyModuleAttributeToFunction(Function &F, StringRef AttrName,
-                              std::pair<StringRef, StringRef> Values) {
-  CopyModuleAttributeToFunction(F, AttrName, AttrName, Values);
-}
-
 void llvm::CopyModuleAttrToFunctions(Module &M) {
   Triple T(M.getTargetTriple());
   if (!T.isThumb() && !T.isARM() && !T.isAArch64())
     return;
 
+  StringRef SignTypeValue = "none";
+  if (isModuleAttributeSet(M, "sign-return-address"))
+    SignTypeValue = "non-leaf";
+  if (isModuleAttributeSet(M, "sign-return-address-all"))
+    SignTypeValue = "all";
+
+  StringRef BTEValue =
+      isModuleAttributeSet(M, "branch-target-enforcement") ? "true" : "false";
+  StringRef BPPLValue =
+      isModuleAttributeSet(M, "branch-protection-pauth-lr") ? "true" : "false";
+  StringRef GCSValue =
+      isModuleAttributeSet(M, "guarded-control-stack") ? "true" : "false";
+  StringRef SignKeyValue =
+      isModuleAttributeSet(M, "sign-return-address-key") ? "b_key" : "a_key";
+
   for (Function &F : M.getFunctionList()) {
     if (F.isDeclaration())
       continue;
+    auto SetFunctionAttrIfNotSet = [&](StringRef FnAttrName, StringRef Value) {
+      if (!F.hasFnAttribute(FnAttrName))
+        F.addFnAttr(FnAttrName, Value);
+    };
 
-    if (!F.hasFnAttribute("sign-return-address")) {
-      StringRef SignType = "none";
-      if (isModuleAttributeSet(M, "sign-return-address"))
-        SignType = "non-leaf";
-
-      if (isModuleAttributeSet(M, "sign-return-address-all"))
-        SignType = "all";
-
-      F.addFnAttr("sign-return-address", SignType);
-    }
-    CopyModuleAttributeToFunction(F, "branch-target-enforcement");
-    CopyModuleAttributeToFunction(F, "branch-protection-pauth-lr");
-    CopyModuleAttributeToFunction(F, "guarded-control-stack");
-    CopyModuleAttributeToFunction(
-        F, "sign-return-address-key",
-        std::make_pair<StringRef, StringRef>("b_key", "a_key"));
+    SetFunctionAttrIfNotSet("sign-return-address", SignTypeValue);
+    SetFunctionAttrIfNotSet("branch-target-enforcement", BTEValue);
+    SetFunctionAttrIfNotSet("branch-protection-pauth-lr", BPPLValue);
+    SetFunctionAttrIfNotSet("guarded-control-stack", GCSValue);
+    SetFunctionAttrIfNotSet("sign-return-address-key", SignKeyValue);
   }
 }

nikic

Looks reasonable

github-actions · 2024-03-08T15:59:36Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/IR/AutoUpgrade.cpp

nickdesaulniers · 2024-03-08T17:30:45Z

llvm/lib/IR/AutoUpgrade.cpp

+  if (isModuleAttributeSet(M, "sign-return-address"))
+    SignTypeValue = "non-leaf";
+  if (isModuleAttributeSet(M, "sign-return-address-all"))


Unhelpful comment orthogonal to this PR:

It seems that the module attributes are either:

sign-return-address (i32 0 or 1)

sign-return-address-all (i32 0 or 1)

But then the function attribute is:

sign-return-address (string "none", "non-leaf", or "all")

Is that correct? If so, there's something about that asymmetry that is grinding.

sign-return-address 0 / sign-return-address-all 0 -> "none"
sign-return-address 0 / sign-return-address-all 0 -> "non-leaf"
sign-return-address 1 / sign-return-address-all 1 -> "all"
sign-return-address 0 / sign-return-address-all 1 -> "invalid". - on my list of missing validations.

I didn't find they not a just 0,1,2 are used for the module values with direct "none", "non-leaf", or "all" mapping.

nickdesaulniers · 2024-03-08T17:33:00Z

Looks like the build bot timed out. You may need to push to rekick it. Or you might be able to in the buildkite UI.

DanielKristofKiss · 2024-03-08T21:26:30Z

llvm/lib/IR/AutoUpgrade.cpp

-    CopyModuleAttributeToFunction(F, "branch-protection-pauth-lr");
-    CopyModuleAttributeToFunction(F, "guarded-control-stack");
-    CopyModuleAttributeToFunction(
-        F, "sign-return-address-key",


the module flag is "sign-return-address-with-bkey" instead of "sign-return-address-key".
fixed now.

err...was that a recently introduced bug? Do we need to backport that specific fix (and thus the test modification)? That makes this change no long NFC.

The bug is not yet backported, but due to performance the original PR should go with this change anyway.

DanielKristofKiss · 2024-03-11T18:01:51Z

Once all patches around the flag handling landed we can further reduce the work here #84804 so only would do any work if we import an older version the IR.

nickdesaulniers · 2024-03-12T20:19:59Z

So I think @dhoekwater and @pranavk have pointed out where the suspected reported regression may have come from in the base commit.

What happens when a module never had these module level metadata set? There's a difference between "sign-return-address"=0 and <nothing>. They suspect what's happening is that modules with are being linked against other modules, which is then setting sign-return-address="false" on the second module, which is now no longer being inlined.

DanielKristofKiss · 2024-03-13T15:38:31Z

So I think @dhoekwater and @pranavk have pointed out where the suspected reported regression may have come from in the base commit.

If you have something that I can reproduce I'd try to debug it too.

What happens when a module never had these module level metadata set? There's a difference between "sign-return-address"=0 and <nothing>. They suspect what's happening is that modules with are being linked against other modules, which is then setting sign-return-address="false" on the second module, which is now no longer being inlined.

All module goes thru here so we should not have mix of "sign-return-address"=0 and <nothing> in the merged module.

pranavk · 2024-03-13T22:59:46Z

Minimal reproducer:

file1.c

int foo();

int main() {
        return foo();
}

file2.c

int foo() {
        return 32;
}

$CLANG -flto=thin -target aarch64-none-linux-gnu -O2 file1.c file2.c -c
$CLANG -fuse-ld=lld -target aarch64-none-linux-gnu -Wl,-plugin-opt,save-temps -flto=thin -O2 file1.o file2.o -o a.out

foo used to get inlined earlier, not anymore because attributes are different now. This is what I believe is the cause of severe performance regression. Up to 30% degradation on multiple benchmarks.

dhoekwater · 2024-03-13T23:10:44Z

llvm/lib/IR/AutoUpgrade.cpp

@@ -5182,65 +5182,46 @@ void llvm::UpgradeFunctionAttributes(Function &F) {
 static bool isModuleAttributeSet(Module &M, const StringRef &ModAttr) {


Currently, this function treats "Attribute X is set with value 0" the same as "Attribute X is unset" for a given module.

This behavior causes functions within modules with no attributes to be annotated with the sign-return-address, branch-target-enforcement, branch-protection-pauth-lr, guarded-control-stack, and sign-return-address-key attributes.

dhoekwater · 2024-03-13T23:17:11Z

llvm/lib/IR/AutoUpgrade.cpp

 }

 void llvm::CopyModuleAttrToFunctions(Module &M) {
  Triple T(M.getTargetTriple());
  if (!T.isThumb() && !T.isARM() && !T.isAArch64())
    return;

+  StringRef SignTypeValue = "none";


Since adding the sign-return-address=none attribute is not a noop (it should not be set if pac and bti are disabled), is there something we can predicate adding this attribute on? For example, is sign-return-address only set when sign-return-address-with-bkey is non-null for the module?

Answering my own question, it looks like uses of this function attribute fall back to the module's attribute if sign-return-address is not set. You should then be able to only set this module's value if it is all or non-leaf.

@dhoekwater I have a few changes that changes this behaviour to set flag only when enabled so if all dependency merged then the module flag won't be used anymore for function attribute so here could set only the positive flags.
also run the code only when the LTO object is from an older frontend.
#84804

dhoekwater · 2024-03-14T02:14:30Z

llvm/lib/IR/AutoUpgrade.cpp

+      isModuleAttributeSet(M, "branch-protection-pauth-lr") ? "true" : "false";
+  StringRef GCSValue =
+      isModuleAttributeSet(M, "guarded-control-stack") ? "true" : "false";
+  StringRef SignKeyValue =


It looks like sign-return-address-key is only present if sign-return-address is all or non-leaf. You should be able to do the same thing here and avoid setting this function attribute in those cases.

dhoekwater · 2024-03-14T02:16:51Z

llvm/lib/IR/AutoUpgrade.cpp

 }

 void llvm::CopyModuleAttrToFunctions(Module &M) {
  Triple T(M.getTargetTriple());
  if (!T.isThumb() && !T.isARM() && !T.isAArch64())
    return;

+  StringRef SignTypeValue = "none";


Answering my own question, it looks like uses of this function attribute fall back to the module's attribute if sign-return-address is not set. You should then be able to only set this module's value if it is all or non-leaf.

nickdesaulniers · 2024-03-14T17:15:32Z

I've added more thoughts here thanks for the test case @pranavk !

…ttributes." (#85291) Reverts #82763 because caused a regressions with inlining. See #84494 (comment)

Refactoring llvm#82763 to cache module attributes.

MaskRay · 2024-03-22T06:01:21Z

lld/test/ELF/lto/aarch64_inline.ll

+;; by the thin-lto linker/IRMover.
+;; Regression test for #82763
+
+; RUN: split-file %s %t


This seems unneeded. You can add ThinLTO tests to llvm/test/ThinLTO

I added the test here because I'd use ld.lld as it invokes the IRMover differently than llvm-lto.
and under llvm/test/ThinLTO ld.lld is not available.
llvm-lto merges modules to an empty module, while ld.lld merges modules into the first module.

Drive by comment: To mimic the ThinLTO behavior of lld use llvm-lto2 not llvm-lto - the former uses the same LTO API (linker resolution-based) whereas the latter uses the legacy one.

Thanks @MaskRay and @teresajohnson !
Moved the test, cheers. (in #86212 )

DanielKristofKiss requested review from nikic, nickdesaulniers and aeubanks March 8, 2024 14:45

llvmbot added the llvm:ir label Mar 8, 2024

nikic approved these changes Mar 8, 2024

View reviewed changes

DanielKristofKiss force-pushed the refactor_autoupgrade branch from 84eae9d to 36544d9 Compare March 8, 2024 15:57

nickdesaulniers reviewed Mar 8, 2024

View reviewed changes

llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Mar 8, 2024

DanielKristofKiss commented Mar 8, 2024

View reviewed changes

DanielKristofKiss changed the title ~~[NFC][LLVM] Refactor Autoupgrade function attributes from Module attributes.~~ [LLVM] Refactor Autoupgrade function attributes from Module attributes. Mar 8, 2024

DanielKristofKiss mentioned this pull request Mar 11, 2024

[ARM][AArch64] Change module flags values. #84804

Closed

nickdesaulniers requested review from pranavk and dhoekwater March 12, 2024 20:15

dhoekwater reviewed Mar 14, 2024

View reviewed changes

dhoekwater requested changes Mar 14, 2024

View reviewed changes

nickdesaulniers mentioned this pull request Mar 14, 2024

[llvm][AArch64] Autoupgrade function attributes from Module attributes. #82763

Merged

DanielKristofKiss mentioned this pull request Mar 14, 2024

Revert "[llvm][AArch64] Autoupgrade function attributes from Module attributes." #85291

Merged

DanielKristofKiss added a commit that referenced this pull request Mar 14, 2024

Revert "[llvm][AArch64] Autoupgrade function attributes from Module a…

4b0276d

…ttributes." (#85291) Reverts #82763 because caused a regressions with inlining. See #84494 (comment)

[LLVM] Autoupgrade function attributes from Module attributes.

8ecca58

Refactoring llvm#82763 to cache module attributes.

DanielKristofKiss force-pushed the refactor_autoupgrade branch from 5a062e9 to 8ecca58 Compare March 19, 2024 08:37

llvmbot added the lld label Mar 19, 2024

llvmbot added the lld:ELF label Mar 19, 2024

DanielKristofKiss mentioned this pull request Mar 19, 2024

[Clang][ARM][AArch64] Alway emit protection attributes for functions. #82819

Open

MaskRay reviewed Mar 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLVM] Refactor Autoupgrade function attributes from Module attributes. #84494

[LLVM] Refactor Autoupgrade function attributes from Module attributes. #84494

DanielKristofKiss commented Mar 8, 2024 •

edited

llvmbot commented Mar 8, 2024 •

edited

nikic left a comment

github-actions bot commented Mar 8, 2024 •

edited

nickdesaulniers Mar 8, 2024

DanielKristofKiss Mar 8, 2024

nickdesaulniers commented Mar 8, 2024

DanielKristofKiss Mar 8, 2024

nickdesaulniers Mar 8, 2024

DanielKristofKiss Mar 8, 2024

DanielKristofKiss commented Mar 11, 2024

nickdesaulniers commented Mar 12, 2024 •

edited by nikic

DanielKristofKiss commented Mar 13, 2024

pranavk commented Mar 13, 2024

dhoekwater Mar 13, 2024

dhoekwater Mar 13, 2024

dhoekwater Mar 14, 2024

DanielKristofKiss Mar 19, 2024

dhoekwater Mar 14, 2024

dhoekwater Mar 14, 2024

nickdesaulniers commented Mar 14, 2024

MaskRay Mar 22, 2024

DanielKristofKiss Mar 22, 2024

teresajohnson Mar 22, 2024

DanielKristofKiss Mar 22, 2024 •

edited

		@@ -5182,65 +5182,46 @@ void llvm::UpgradeFunctionAttributes(Function &F) {
		static bool isModuleAttributeSet(Module &M, const StringRef &ModAttr) {

[LLVM] Refactor Autoupgrade function attributes from Module attributes. #84494

Are you sure you want to change the base?

[LLVM] Refactor Autoupgrade function attributes from Module attributes. #84494

Conversation

DanielKristofKiss commented Mar 8, 2024 • edited

llvmbot commented Mar 8, 2024 • edited

nikic left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielKristofKiss commented Mar 11, 2024

nickdesaulniers commented Mar 12, 2024 • edited by nikic

DanielKristofKiss commented Mar 13, 2024

pranavk commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielKristofKiss Mar 22, 2024 • edited

Choose a reason for hiding this comment

DanielKristofKiss commented Mar 8, 2024 •

edited

llvmbot commented Mar 8, 2024 •

edited

github-actions bot commented Mar 8, 2024 •

edited

nickdesaulniers commented Mar 12, 2024 •

edited by nikic

DanielKristofKiss Mar 22, 2024 •

edited