[FunctionAttrs][ArgumentPromotion][GlobalOpt] Disable some optimisations passes for naked functions

[LukeCheeseman]

Summary

• Fix for bug 36078.
• Prevent the functionattrs, function-attrs, globalopt and argpromotion passes from changing naked functions.
• These passes can perform some alterations to the functions that should not be applied. An example is removing parameters that are seemingly not used because they are only referenced in the inline assembly. Another example is marking the function as fastcc.

[rnk]

lgtm

We might want to consider some kind of helper on Function that checks for optnone and naked to block these kinds of prototype-changing optimizations.

[LukeCheeseman]

This suggestion makes sense to me. However, I have been looking into optnone and naked and I'm not sure whether they apply in the same way for inter procedural optimisations after reading https://reviews.llvm.org/D1288. An example of this is as follows:

$ cat test.ll
define internal i32 @foo(i32*) #0 {
entry:
  %retval = alloca i32, align 4
  unreachable
}

attributes #0 = { optnone noinline }

$ /work/llvm/svn+release/bin/opt -deadargelim naked.ll -S
; ModuleID = 'test.ll'
source_filename = "test.ll"

; Function Attrs: noinline optnone
define internal void @foo() #0 {
entry:
  %retval = alloca i32, align 4
  unreachable
}

attributes #0 = { noinline optnone }

The argument from foo has been removed, whereas if we make the function as naked the result is different:

$ /work/llvm/svn+release/bin/opt -deadargelim naked.ll -S 
; ModuleID = 'test.ll'
source_filename = "test.ll"

; Function Attrs: naked
define internal i32 @foo(i32*) #0 {
entry:
  %retval = alloca i32, align 4
  unreachable
}

attributes #0 = { naked }

I'm not sure whether this is a bug in deadargument elimination or is the expected behavior of optnone. Reading the description of optnone I would think it was the latter but this is reading a description from 5 years ago, in which case I'm not sure we would be able to introduce such a helper. The clang documentation on optnone is not explicit as to whether the prototypes are subject to change or not.

I'd like to hear your thoughts on this.

lib/Transforms/IPO/ArgumentPromotion.cpp

 /// are any promotable arguments and if it is safe to promote the function (for
 /// example, all callers are direct).  If safe to promote some arguments, it
 /// calls the DoPromotion method.
 static Function *
 promoteArguments(Function *F, function_ref<AAResults &(Function &F)> AARGetter,
                  unsigned MaxElements,
                  Optional<function_ref<void(CallSite OldCS, CallSite NewCS)>>
                      ReplaceCallSite) {
+  // Don't perform argument promotion for naked functions; otherwise we can end
+  // up removing parameters that are seemingly 'not used' as they are referred
+  // to in the assembly.
+  if(F->hasFnAttribute(Attribute::Naked))
+    return nullptr;
+
   // Make sure that it is local to this module.
   if (!F->hasLocalLinkage())
     return nullptr;
 
   // Don't promote arguments for variadic functions. Adding, removing, or
   // changing non-pack parameters can change the classification of pack
   // parameters. Frontends encode that classification at the call site in the
   // IR, while in the callee the classification is determined dynamically based

lib/Transforms/IPO/FunctionAttrs.cpp

PreservedAnalyses PostOrderFunctionAttrsPass::run(LazyCallGraph::SCC &C,
 
   // Fill SCCNodes with the elements of the SCC. Also track whether there are
   // any external or opt-none nodes that will prevent us from optimizing any
   // part of the SCC.
   SCCNodeSet SCCNodes;
   bool HasUnknownCall = false;
   for (LazyCallGraph::Node &N : C) {
     Function &F = N.getFunction();
-    if (F.hasFnAttribute(Attribute::OptimizeNone)) {
+    if (F.hasFnAttribute(Attribute::OptimizeNone) ||
+        F.hasFnAttribute(Attribute::Naked)) {
       // Treat any function we're trying not to optimize as if it were an
       // indirect call and omit it from the node set used below.
       HasUnknownCall = true;
       continue;
     }
     // Track whether any functions in this SCC have an unknown call edge.
     // Note: if this is ever a performance hit, we can common it with
     // subsequent routines which also do scans over the instructions of the
static bool runImpl(CallGraphSCC &SCC, AARGetterT AARGetter) {
   // Fill SCCNodes with the elements of the SCC. Used for quickly looking up
   // whether a given CallGraphNode is in this SCC. Also track whether there are
   // any external or opt-none nodes that will prevent us from optimizing any
   // part of the SCC.
   SCCNodeSet SCCNodes;
   bool ExternalNode = false;
   for (CallGraphNode *I : SCC) {
     Function *F = I->getFunction();
-    if (!F || F->hasFnAttribute(Attribute::OptimizeNone)) {
+    if (!F || F->hasFnAttribute(Attribute::OptimizeNone) ||
+        F->hasFnAttribute(Attribute::Naked)) {
       // External node or function we're trying not to optimize - we both avoid
       // transform them and avoid leveraging information they provide.
       ExternalNode = true;
       continue;
     }
 
     SCCNodes.insert(F);
   }

lib/Transforms/IPO/GlobalOpt.cpp

for (Module::iterator FI = M.begin(), E = M.end(); FI != E;) {
     if (hasOnlyColdCalls(*F, GetBFI))
       AllCallsCold.push_back(F);
   }
 
   // Optimize functions.
   for (Module::iterator FI = M.begin(), E = M.end(); FI != E; ) {
     Function *F = &*FI++;
 
+    // Don't perform global opt pass on naked functions; we don't want fast
+    // calling conventions for naked functions.
+    if (F->hasFnAttribute(Attribute::Naked))
+      continue;
+
     // Functions without names cannot be referenced outside this module.
     if (!F->hasName() && !F->isDeclaration() && !F->hasLocalLinkage())
       F->setLinkage(GlobalValue::InternalLinkage);
 
     if (deleteIfDead(*F, NotDiscardableComdats)) {
       Changed = true;
       continue;
     }

test/Transforms/ArgumentPromotion/naked_functions.ll

+; RUN: opt < %s -argpromotion -S | FileCheck %s
+
+; Don't promote paramaters of/arguments to naked functions
+
+@g = common global i32 0, align 4
+
+define i32 @bar() {
+entry:
+  %call = call i32 @foo(i32* @g)
+; CHECK: %call = call i32 @foo(i32* @g)
+  ret i32 %call
+}
+
+define internal i32 @foo(i32*) #0 {
+entry:
+  %retval = alloca i32, align 4
+  call void asm sideeffect "ldr r0, [r0] \0Abx lr        \0A", ""()
+  unreachable
+}
+
+; CHECK: define internal i32 @foo(i32*)
+
+attributes #0 = { naked }

test/Transforms/FunctionAttrs/naked_functions.ll

+; RUN: opt -S -functionattrs %s | FileCheck %s
+; RUN: opt -S -passes='function-attrs' %s | FileCheck %s
+
+; Don't change the attributes of parameters of naked functions, in particular
+; don't mark them as readnone
+
+@g = common global i32 0, align 4
+
+define i32 @bar() {
+entry:
+  %call = call i32 @foo(i32* @g)
+; CHECK: %call = call i32 @foo(i32* @g)
+  ret i32 %call
+}
+
+define internal i32 @foo(i32*) #0 {
+entry:
+  %retval = alloca i32, align 4
+  call void asm sideeffect "ldr r0, [r0] \0Abx lr        \0A", ""()
+  unreachable
+}
+
+; CHECK: define internal i32 @foo(i32*)
+
+attributes #0 = { naked }

test/Transforms/GlobalOpt/naked_functions.ll

+; RUN: opt < %s -globalopt -S | FileCheck %s
+
+; Check that naked functions don't get marked with fast calling conventions
+
+@g = common global i32 0, align 4
+
+define i32 @bar() {
+entry:
+  %call = call i32 @foo(i32* @g)
+; CHECK: %call = call i32 @foo(i32* @g)
+  ret i32 %call
+}
+
+define internal i32 @foo(i32*) #0 {
+entry:
+  %retval = alloca i32, align 4
+  call void asm sideeffect "ldr r0, [r0] \0Abx lr        \0A", ""()
+  unreachable
+}
+
+; CHECK: define internal i32 @foo(i32*)
+
+attributes #0 = { naked }