[attrs] Split off the forced attributes utility into its own pass that can be potentially run much earlier than FuncitonAttrs proper.

[chandlerc]

Summary

This, for example, would allow forcing optnone or other widely impactful
attribute. It is also a bit simpler as the force attribute behavior needs no
specific iteration order.

An important question is whether it is useful to add this to the default pass
pipelines or not. I'm indifferent, happy to take suggestions. If so, it should
go at the very start so that optnone takes full effect, etc.

[chandlerc]

Finish adding support for the forceattrs pass to the new pass manager and test it.

[sanjoy]

Added some minor drive by comments inline.

include/llvm/Transforms/IPO/ForceFunctionAttrs.h

11

The comments seem out of sync (possibly from StripDeadPrototypes)?

lib/Transforms/IPO/ForceFunctionAttrs.cpp

82

Minor, but might be clearer as

if (!F.hasFnAttribute(Kind))
  F.addFnAttr(Kind);

(unless you plan to add more things to the non continue; branch later)

[chandlerc]

include/llvm/Transforms/IPO/ForceFunctionAttrs.h

11

Yea, sorry, will fix them to be correct.

lib/Transforms/IPO/ForceFunctionAttrs.cpp

82

Just moving code around here. I can clean it up in a follow-up patch?

[majnemer]

include/llvm/Transforms/IPO/ForceFunctionAttrs.h

2

Shouldn't this be 80 col long to match?

lib/Transforms/IPO/ForceFunctionAttrs.cpp

16–21

Please sort these.

[sanjoy]

lib/Transforms/IPO/ForceFunctionAttrs.cpp

82

sgtm.

[chandlerc]

Run clang-format, fix up comments.

[chandlerc]

All comments addressed.

[probinson]

One nit inline. Fixing it as a followup would be fine.

Also it makes me vaguely uncomfortable that the list of forceable attributes is buried in this piece of code. I guess it's for debugging/testing only, so something more formal isn't really warranted?

lib/Transforms/IPO/ForceFunctionAttrs.cpp

52

I see it's a cut-and-paste but this one is out of order.

[chandlerc]

I didn't introduce this functionality, so I'm not particularly invested in its current design, but I'd like to save that discussion for later. I'd suggest something on llvm-dev and CC-ing the author. The question is, does this refactoring make sense given that the functionality is already in the tree...

lib/Transforms/IPO/ForceFunctionAttrs.cpp

52

Yea, I'd like to leave cleanups of this for follow-up. I really just want to move this code first and make sure folks are comfortable with that.

[jmolloy]

Generally LGTM, but I *really* would like to see this in the default pipeline. Its primary purpose is to allow users to fiddle around with the heuristics a bit (alwaysinline/noinline are probably the most useful attributes we have), and given how negligible the performance impact of this is I'm very much for it being enabled by default so users didn't have to recompile their compiler to do some heuristic exploration.

Aside:

I'd suggest something on llvm-dev and CC-ing the author.

As the author, I can reply here :) I'm not particularly invested either in this parse function and didn't want to do it this way. What there ideally should be is some method on Attribute to parse the textual form of an enum attribute. That's not currently possible - you get a String attribute without an associated enum number.

The attribute parsing logic lives in the AsmParser, so extracting it out of there into Attribute would seem to me the right thing to do longer term. For this patch, it was simply for debugging / testing / exploration and the duplicated code was limited, so it didn't seem worth the refactoring investment.

I can however reconsider this if people think it's worth it :)

[chandlerc]

Thanks James, I've added it to the pass manager builder in comparable places and I'm submitting. Feel free to move around if you like!

include/llvm/InitializePasses.h

 void initializeMemorySanitizerPass(PassRegistry&);
 void initializeThreadSanitizerPass(PassRegistry&);
 void initializeSanitizerCoverageModulePass(PassRegistry&);
 void initializeDataFlowSanitizerPass(PassRegistry&);
 void initializeScalarizerPass(PassRegistry&);
 void initializeEarlyCSELegacyPassPass(PassRegistry &);
 void initializeEliminateAvailableExternallyPass(PassRegistry&);
 void initializeExpandISelPseudosPass(PassRegistry&);
+void initializeForceFunctionAttrsLegacyPassPass(PassRegistry&);
 void initializeFunctionAttrsPass(PassRegistry&);
 void initializeGCMachineCodeAnalysisPass(PassRegistry&);
 void initializeGCModuleInfoPass(PassRegistry&);
 void initializeGVNPass(PassRegistry&);
 void initializeGlobalDCEPass(PassRegistry&);
 void initializeGlobalOptPass(PassRegistry&);
 void initializeGlobalsAAWrapperPassPass(PassRegistry&);
 void initializeIPCPPass(PassRegistry&);

include/llvm/Transforms/IPO/ForceFunctionAttrs.h

+//===-- ForceFunctionAttrs.h - Remove unused function declarations -------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This pass loops over all of the functions in the input module, looking for
+// dead declarations and removes them. Dead declarations are declarations of
+// functions for which no implementation is available (i.e., declarations for
+// unused library functions).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_TRANSFORMS_IPO_FORCEFUNCTIONATTRS_H
+#define LLVM_TRANSFORMS_IPO_FORCEFUNCTIONATTRS_H
+
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PassManager.h"
+
+namespace llvm {
+
+/// Pass which forces specific function attributes into the IR, primarily as
+/// a debugging tool.
+class ForceFunctionAttrsPass {
+public:
+  static StringRef name() { return "ForceFunctionAttrsPass"; }
+  PreservedAnalyses run(Module &M);
+};
+
+/// Create a legacy pass manager instance of a pass to force function attrs.
+Pass *createForceFunctionAttrsLegacyPass();
+
+}
+
+#endif // LLVM_TRANSFORMS_IPO_FORCEFUNCTIONATTRS_H

lib/Transforms/IPO/CMakeLists.txt

 add_llvm_library(LLVMipo
   ArgumentPromotion.cpp
   BarrierNoopPass.cpp
   ConstantMerge.cpp
   CrossDSOCFI.cpp
   DeadArgumentElimination.cpp
   ElimAvailExtern.cpp
   ExtractGV.cpp
+  ForceFunctionAttrs.cpp
   FunctionAttrs.cpp
   FunctionImport.cpp
   GlobalDCE.cpp
   GlobalOpt.cpp
   IPConstantPropagation.cpp
   IPO.cpp
   InlineAlways.cpp
   InlineSimple.cpp

lib/Transforms/IPO/ForceFunctionAttrs.cpp

+//===- ForceFunctionAttrs.cpp - Pass to force function attrs for debugging-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+/// \file
+/// Super simple passes to force specific function attrs from the commandline
+/// into the IR for debugging purposes.
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Transforms/IPO/ForceFunctionAttrs.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/ADT/StringSwitch.h"
+#include "llvm/Support/raw_ostream.h"
+using namespace llvm;
+
+#define DEBUG_TYPE "forceattrs"
+
+static cl::list<std::string>
+ForceAttributes("force-attribute", cl::Hidden,
+                cl::desc("Add an attribute to a function. This should be a "
+                         "pair of 'function-name:attribute-name', for "
+                         "example -force-add-attribute=foo:noinline. This "
+                         "option can be specified multiple times."));
+
+static Attribute::AttrKind parseAttrKind(StringRef Kind) {
+  return StringSwitch<Attribute::AttrKind>(Kind)
+    .Case("alwaysinline", Attribute::AlwaysInline)
+    .Case("builtin", Attribute::Builtin)
+    .Case("cold", Attribute::Cold)
+    .Case("convergent", Attribute::Convergent)
+    .Case("inlinehint", Attribute::InlineHint)
+    .Case("jumptable", Attribute::JumpTable)
+    .Case("minsize", Attribute::MinSize)
+    .Case("naked", Attribute::Naked)
+    .Case("nobuiltin", Attribute::NoBuiltin)
+    .Case("noduplicate", Attribute::NoDuplicate)
+    .Case("noimplicitfloat", Attribute::NoImplicitFloat)
+    .Case("noinline", Attribute::NoInline)
+    .Case("nonlazybind", Attribute::NonLazyBind)
+    .Case("noredzone", Attribute::NoRedZone)
+    .Case("noreturn", Attribute::NoReturn)
+    .Case("norecurse", Attribute::NoRecurse)
+    .Case("nounwind", Attribute::NoUnwind)
+    .Case("optnone", Attribute::OptimizeNone)
+    .Case("optsize", Attribute::OptimizeForSize)
+    .Case("readnone", Attribute::ReadNone)
+    .Case("readonly", Attribute::ReadOnly)
+    .Case("argmemonly", Attribute::ArgMemOnly)
+    .Case("returns_twice", Attribute::ReturnsTwice)
+    .Case("safestack", Attribute::SafeStack)
+    .Case("sanitize_address", Attribute::SanitizeAddress)
+    .Case("sanitize_memory", Attribute::SanitizeMemory)
+    .Case("sanitize_thread", Attribute::SanitizeThread)
+    .Case("ssp", Attribute::StackProtect)
+    .Case("sspreq", Attribute::StackProtectReq)
+    .Case("sspstrong", Attribute::StackProtectStrong)
+    .Case("uwtable", Attribute::UWTable)
+    .Default(Attribute::None);
+}
+
+/// If F has any forced attributes given on the command line, add them.
+static void addForcedAttributes(Function &F) {
+  for (auto &S : ForceAttributes) {
+    auto KV = StringRef(S).split(':');
+    if (KV.first != F.getName())
+      continue;
+
+    auto Kind = parseAttrKind(KV.second);
+    if (Kind == Attribute::None) {
+      DEBUG(dbgs() << "ForcedAttribute: " << KV.second
+                   << " unknown or not handled!\n");
+      continue;
+    }
+    if (F.hasFnAttribute(Kind))
+      continue;
+    F.addFnAttr(Kind);
+  }
+}
+
+PreservedAnalyses
+ForceFunctionAttrsPass::run(Module &M) {
+  if (ForceAttributes.empty())
+    return PreservedAnalyses::all();
+
+  for (Function &F : M.functions())
+    addForcedAttributes(F);
+
+  // Just conservatively invalidate analyses, this isn't likely to be important.
+  return PreservedAnalyses::none();
+}
+
+namespace {
+struct ForceFunctionAttrsLegacyPass : public ModulePass {
+  static char ID; // Pass identification, replacement for typeid
+  ForceFunctionAttrsLegacyPass() : ModulePass(ID) {
+    initializeForceFunctionAttrsLegacyPassPass(
+        *PassRegistry::getPassRegistry());
+  }
+
+  bool runOnModule(Module &M) override {
+    if (ForceAttributes.empty())
+      return false;
+
+    for (Function &F : M.functions())
+      addForcedAttributes(F);
+
+    // Conservatively assume we changed something.
+    return true;
+  }
+};
+}
+
+char ForceFunctionAttrsLegacyPass::ID = 0;
+INITIALIZE_PASS(ForceFunctionAttrsLegacyPass, "forceattrs",
+                "Force set function attributes", false, false)
+
+Pass *llvm::createForceFunctionAttrsLegacyPass() {
+  return new ForceFunctionAttrsLegacyPass();
+}
+

lib/Transforms/IPO/FunctionAttrs.cpp

 STATISTIC(NumNoCapture, "Number of arguments marked nocapture");
 STATISTIC(NumReadNoneArg, "Number of arguments marked readnone");
 STATISTIC(NumReadOnlyArg, "Number of arguments marked readonly");
 STATISTIC(NumNoAlias, "Number of function returns marked noalias");
 STATISTIC(NumNonNullReturn, "Number of function returns marked nonnull");
 STATISTIC(NumAnnotated, "Number of attributes added to library functions");
 STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");
 
-static cl::list<std::string>
-ForceAttributes("force-attribute", cl::Hidden,
-                cl::desc("Add an attribute to a function. This should be a "
-                         "pair of 'function-name:attribute-name', for "
-                         "example -force-add-attribute=foo:noinline. This "
-                         "option can be specified multiple times."));
-
 namespace {
 typedef SmallSetVector<Function *, 8> SCCNodeSet;
 }
 
 namespace {
 struct FunctionAttrs : public CallGraphSCCPass {
   static char ID; // Pass identification, replacement for typeid
   FunctionAttrs() : CallGraphSCCPass(ID) {
for (auto *U : F->users())
       } else {
         return false;
       }
     return setDoesNotRecurse(*F);
   }
   return false;
 }
 
-static Attribute::AttrKind parseAttrKind(StringRef Kind) {
-  return StringSwitch<Attribute::AttrKind>(Kind)
-    .Case("alwaysinline", Attribute::AlwaysInline)
-    .Case("builtin", Attribute::Builtin)
-    .Case("cold", Attribute::Cold)
-    .Case("convergent", Attribute::Convergent)
-    .Case("inlinehint", Attribute::InlineHint)
-    .Case("jumptable", Attribute::JumpTable)
-    .Case("minsize", Attribute::MinSize)
-    .Case("naked", Attribute::Naked)
-    .Case("nobuiltin", Attribute::NoBuiltin)
-    .Case("noduplicate", Attribute::NoDuplicate)
-    .Case("noimplicitfloat", Attribute::NoImplicitFloat)
-    .Case("noinline", Attribute::NoInline)
-    .Case("nonlazybind", Attribute::NonLazyBind)
-    .Case("noredzone", Attribute::NoRedZone)
-    .Case("noreturn", Attribute::NoReturn)
-    .Case("norecurse", Attribute::NoRecurse)
-    .Case("nounwind", Attribute::NoUnwind)
-    .Case("optnone", Attribute::OptimizeNone)
-    .Case("optsize", Attribute::OptimizeForSize)
-    .Case("readnone", Attribute::ReadNone)
-    .Case("readonly", Attribute::ReadOnly)
-    .Case("argmemonly", Attribute::ArgMemOnly)
-    .Case("returns_twice", Attribute::ReturnsTwice)
-    .Case("safestack", Attribute::SafeStack)
-    .Case("sanitize_address", Attribute::SanitizeAddress)
-    .Case("sanitize_memory", Attribute::SanitizeMemory)
-    .Case("sanitize_thread", Attribute::SanitizeThread)
-    .Case("ssp", Attribute::StackProtect)
-    .Case("sspreq", Attribute::StackProtectReq)
-    .Case("sspstrong", Attribute::StackProtectStrong)
-    .Case("uwtable", Attribute::UWTable)
-    .Default(Attribute::None);
-}
-
-/// If F has any forced attributes given on the command line, add them.
-static bool addForcedAttributes(Function *F) {
-  bool Changed = false;
-  for (auto &S : ForceAttributes) {
-    auto KV = StringRef(S).split(':');
-    if (KV.first != F->getName())
-      continue;
-
-    auto Kind = parseAttrKind(KV.second);
-    if (Kind == Attribute::None) {
-      DEBUG(dbgs() << "ForcedAttribute: " << KV.second
-                   << " unknown or not handled!\n");
-      continue;
-    }
-    if (F->hasFnAttribute(Kind))
-      continue;
-    Changed = true;
-    F->addFnAttr(Kind);
-  }
-  return Changed;
-}
-
 bool FunctionAttrs::runOnSCC(CallGraphSCC &SCC) {
   TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
   bool Changed = false;
 
   // We compute dedicated AA results for each function in the SCC as needed. We
   // use a lambda referencing external objects so that they live long enough to
   // be queried, but we re-use them each time.
   Optional<BasicAAResult> BAR;
if (!F || F->hasFnAttribute(Attribute::OptimizeNone)) {
       continue;
     }
 
     // When initially processing functions, also infer their prototype
     // attributes if they are declarations.
     if (F->isDeclaration())
       Changed |= inferPrototypeAttributes(*F, *TLI);
 
-    Changed |= addForcedAttributes(F);
     SCCNodes.insert(F);
   }
 
   Changed |= addReadAttrs(SCCNodes, AARGetter);
   Changed |= addArgumentAttrs(SCCNodes);
 
   // If we have no external nodes participating in the SCC, we can infer some
   // more precise attributes as well.

lib/Transforms/IPO/IPO.cpp

 using namespace llvm;
 
 void llvm::initializeIPO(PassRegistry &Registry) {
   initializeArgPromotionPass(Registry);
   initializeConstantMergePass(Registry);
   initializeCrossDSOCFIPass(Registry);
   initializeDAEPass(Registry);
   initializeDAHPass(Registry);
+  initializeForceFunctionAttrsLegacyPassPass(Registry);
   initializeFunctionAttrsPass(Registry);
   initializeGlobalDCEPass(Registry);
   initializeGlobalOptPass(Registry);
   initializeIPCPPass(Registry);
   initializeAlwaysInlinerPass(Registry);
   initializeSimpleInlinerPass(Registry);
   initializeInternalizePassPass(Registry);
   initializeLoopExtractorPass(Registry);

test/Transforms/ForcedFunctionAttrs/forced.ll

+; RUN: opt < %s -S -forceattrs | FileCheck %s --check-prefix=CHECK-CONTROL
+; RUN: opt < %s -S -forceattrs -force-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
+
+; CHECK-CONTROL: define void @foo() {
+; CHECK-FOO: define void @foo() #0 {
+define void @foo() {
+  ret void
+}
+
+
+; CHECK-FOO: attributes #0 = { noinline }

test/Transforms/FunctionAttrs/forced.ll

-; RUN: opt < %s -S -functionattrs | FileCheck %s --check-prefix=CHECK-CONTROL
-; RUN: opt < %s -S -functionattrs -force-attribute foo:noinline | FileCheck %s --check-prefix=CHECK-FOO
-
-; CHECK-CONTROL: define void @foo() #0 {
-; CHECK-FOO: define void @foo() #0 {
-define void @foo() {
-  ret void
-}
-
-
-; CHECK-CONTROL: attributes #0 = { norecurse readnone }
-; CHECK-FOO: attributes #0 = { noinline norecurse readnone }