[PATCH] Stop resetting NoFramePointerElim in TargetMachine::resetTargetOptions

[ahatanak]

Summary

This is part of the work to remove TargetMachine::resetTargetOptions.

In this patch, instead of updating global variable TargetOptions::NoFramePointerElim in resetTargetOptions, its use in DisableFramePointerElim is replaced with calls to TargetOptions::noFramePointerElim and TargetFrameLowering::noFramePointerElim. These functions determine on a per-function basis if frame pointer elimination should be disabled.

There is no change in functionality except that cl:opt option "disable-fp-elim" can now override function attribute "no-frame-pointer-elim". I removed the option from test case 2014-08-29-CompactUnwind.ll to prevent it from changing the output (it had no effect previously, but with this patch, it overrides the function attribute).

[echristo]

Lot of extra work just to maintain the command line flag, but it looks fine. Go ahead and commit the X86 testcase fix separately if it is NFC.

-eric

[ahatanak]

Thanks, I'll commit this patch today unless there are further comments.

We can remove NoFramePointerElimOverride if we don't need the cl::opt option. We can also remove NoFramePointerElim if we can assume front-ends or JITs today emit function attribute "no-frame-pointer-elim" to disable frame pointer elimination rather than setting NoFramePointerElim, but I don't think that is the case.

[echristo]

We can probably make the assumption that front ends do that these days
since, of course, there aren't a lot of public front ends and clang has
been doing this for a while.

-eric

[ahatanak]

Does that include JITs too? r180229 made changes in ExecutionEngineBindings.cpp and added an API to enable setting NoFramePointerElim. I wasn't sure if I can make the assumption that users of the API have made changes to use function attributes.

[ahatanak]

I created a new patch which changes the backend to only look at function attribute "no-frame-pointer-elim" and disregard TargetOptions::NoFramePointerElim.

In the new patch, overrideFunctionAttributes is rewritten and renamed. The new function is called from LLVMCreateMCJITCompilerForModule and the tools (llc and opt) and it behaves differently based on where it is called:

• If it is called from the tools, it overwrites function attribute "no-frame-pointer-elim" only if the corresponding command line option "disable-fp-elim" was specified.

• If it is called from LLVMCreateMCJITCompilerForModule, it always writes the function attribute. This is necessary to preserve the behavior of this C API after changes are made in the backend. Note that I'm assuming function attribute "no-frame-pointer-elim" never appears in the module passed to this function as there are no C APIs that allow recording string function attributes as far as I can tell (see include/llvm-c/Core.h).

I also changed it to create and reset the function attribute set once per visited function rather than doing it many times.

I think this patch is better than my previous patch as it simplifies the backend. What do you think?

[echristo]

I like this much more, thank you.

A few inline comments, but don't bother with resubmitting after fixing them or responding, anything else can be follow up.

Thanks a lot!

-eric

include/llvm/CodeGen/CommandFlags.h

233 ↗

(On Diff #26197)

Should be possible to get rid of this now and just check the command line option?

lib/ExecutionEngine/ExecutionEngineBindings.cpp

185 ↗

(On Diff #26197)

Comment the true argument, (and probably why we're not setting the first two arguments as well).

test/CodeGen/X86/2014-08-29-CompactUnwind.ll

1 ↗

(On Diff #26197)

Should probably make this commit separately?

tools/llc/llc.cpp

308 ↗

(On Diff #26197)

Comment the false argument.

tools/opt/opt.cpp

400 ↗

(On Diff #26197)

Comment the false argument.

[ahatanak]

include/llvm/CodeGen/CommandFlags.h

233 ↗

(On Diff #26197)

If we get rid of this, we can't distinguish between the case where "-disable-fp-elim" was not provided at all on the command line and the case where "-disable-fp-elim=false" was provided. For both cases, NoFramePointerElim would evaluate to false.

Or perhaps I'm missing some obvious way to do this?

lib/ExecutionEngine/ExecutionEngineBindings.cpp

185 ↗

(On Diff #26197)

OK, will fix.

test/CodeGen/X86/2014-08-29-CompactUnwind.ll

1 ↗

(On Diff #26197)

Yes, this should be separate. Probably the changes I made to overrideFunctionAttribute can be a separate commit too.

tools/llc/llc.cpp

308 ↗

(On Diff #26197)

Will fix.

[ahatanak]

I think I should just define a separate function that unconditionally sets "no-frame-pointer-elim" and is called only from ExecutionEngineBindings.cpp. I can then rewrite setFunctionAttributes and remove both NoFramePointerElim and NoFramePointerElimOverride from TargetOptions.

I'll probably have a patch ready next week.

[ahatanak]

As a follow-up to r238080, this patch removes NoFramePointerElim and NoFramePointerElimOverride from TargetOptions and removes ExecutionEngine's dependence on CodeGen. I also intend to delete the code in EmitAssemblyHelper::CreateTargetMachine of clang which uses NoFramePointerElim.

[echristo]

This works for me, thanks.

-eric

llvm/trunk/include/llvm/CodeGen/CommandFlags.h

JTableType("jump-table-type",
                          "Create one table per unique function type."),
               clEnumValEnd));
 
 // Common utility function tightly tied to the options listed here. Initializes
 // a TargetOptions object with CodeGen flags and returns it.
 static inline TargetOptions InitTargetOptionsFromCodeGenFlags() {
   TargetOptions Options;
   Options.LessPreciseFPMADOption = EnableFPMAD;
-  Options.NoFramePointerElim = DisableFPElim;
-  Options.NoFramePointerElimOverride = DisableFPElim.getNumOccurrences() > 0;
   Options.AllowFPOpFusion = FuseFPOps;
   Options.UnsafeFPMath = EnableUnsafeFPMath;
   Options.NoInfsFPMath = EnableNoInfsFPMath;
   Options.NoNaNsFPMath = EnableNoNaNsFPMath;
   Options.HonorSignDependentRoundingFPMathOption =
       EnableHonorSignDependentRoundingFPMath;
   if (FloatABIForCalls != FloatABI::Default)
     Options.FloatABIType = FloatABIForCalls;
static inline std::string getFeaturesStr() {
   }
 
   for (unsigned i = 0; i != MAttrs.size(); ++i)
     Features.AddFeature(MAttrs[i]);
 
   return Features.getString();
 }
 
+/// \brief Set function attributes of functions in Module M based on CPU,
+/// Features, and command line flags.
+static inline void setFunctionAttributes(StringRef CPU, StringRef Features,
+                                         Module &M) {
+  for (auto &F : M) {
+    auto &Ctx = F.getContext();
+    AttributeSet Attrs = F.getAttributes(), NewAttrs;
+
+    if (!CPU.empty())
+      NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
+                                       "target-cpu", CPU);
+
+    if (!Features.empty())
+      NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
+                                       "target-features", Features);
+
+    if (DisableFPElim.getNumOccurrences() > 0)
+      NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
+                                       "no-frame-pointer-elim",
+                                       DisableFPElim ? "true" : "false");
+
+    // Let NewAttrs override Attrs.
+    NewAttrs = Attrs.addAttributes(Ctx, AttributeSet::FunctionIndex, NewAttrs);
+    F.setAttributes(NewAttrs);
+  }
+}
+
 #endif

llvm/trunk/include/llvm/Target/TargetOptions.h

enum Model {
       POSIX,  // POSIX Threads
       Single  // Single Threaded Environment
     };
   }
 
   class TargetOptions {
   public:
     TargetOptions()
-        : PrintMachineCode(false), NoFramePointerElim(false),
-          NoFramePointerElimOverride(false),
+        : PrintMachineCode(false),
           LessPreciseFPMADOption(false), UnsafeFPMath(false),
           NoInfsFPMath(false), NoNaNsFPMath(false),
           HonorSignDependentRoundingFPMathOption(false),
           NoZerosInBSS(false),
           GuaranteedTailCallOpt(false),
           DisableTailCalls(false), StackAlignmentOverride(0),
           EnableFastISel(false), PositionIndependentExecutable(false),
           UseInitArray(false), DisableIntegratedAS(false),
           CompressDebugSections(false), FunctionSections(false),
           DataSections(false), UniqueSectionNames(true), TrapUnreachable(false),
           TrapFuncName(), FloatABIType(FloatABI::Default),
           AllowFPOpFusion(FPOpFusion::Standard), JTType(JumpTable::Single),
           ThreadModel(ThreadModel::POSIX) {}
 
     /// PrintMachineCode - This flag is enabled when the -print-machineinstrs
     /// option is specified on the command line, and should enable debugging
     /// output from the code generator.
     unsigned PrintMachineCode : 1;
 
-    /// NoFramePointerElim - This flag is enabled when the -disable-fp-elim is
-    /// specified on the command line.  If the target supports the frame pointer
-    /// elimination optimization, this option should disable it.
-    unsigned NoFramePointerElim : 1;
-
-    /// This flag is true when "disable-fp-elim" appeared on the command line.
-    unsigned NoFramePointerElimOverride : 1;
-
     /// DisableFramePointerElim - This returns true if frame pointer elimination
     /// optimization should be disabled for the given machine function.
     bool DisableFramePointerElim(const MachineFunction &MF) const;
 
     /// LessPreciseFPMAD - This flag is enabled when the
     /// -enable-fp-mad is specified on the command line.  When this flag is off
     /// (the default), the code generator is not allowed to generate mad
     /// (multiply add) if the result is "less precise" than doing those
public:
     /// ThreadModel - This flag specifies the type of threading model to assume
     /// for things like atomics
     ThreadModel::Model ThreadModel;
 
     /// Machine level options.
     MCTargetOptions MCOptions;
   };
 
-/// \brief Set function attributes of functions in Module M based on CPU,
-/// Features, and Options.
-/// If AlwaysRecordAttrs is true, it will always record the function attributes
-/// in Options regardless of whether those attributes were specified on the
-/// tool's command line.
-void setFunctionAttributes(StringRef CPU, StringRef Features,
-                           const TargetOptions &Options, Module &M,
-                           bool AlwaysRecordAttrs);
-
 // Comparison operators:
 
 
 inline bool operator==(const TargetOptions &LHS,
                        const TargetOptions &RHS) {
 #define ARE_EQUAL(X) LHS.X == RHS.X
   return
     ARE_EQUAL(UnsafeFPMath) &&

llvm/trunk/lib/CodeGen/TargetOptionsImpl.cpp

 }
 
 /// getTrapFunctionName - If this returns a non-empty string, this means isel
 /// should lower Intrinsic::trap to a call to the specified function name
 /// instead of an ISD::TRAP node.
 StringRef TargetOptions::getTrapFunctionName() const {
   return TrapFuncName;
 }
-
-
-void llvm::setFunctionAttributes(StringRef CPU, StringRef Features,
-                                 const TargetOptions &Options, Module &M,
-                                 bool AlwaysRecordAttrs) {
-  for (auto &F : M) {
-    auto &Ctx = F.getContext();
-    AttributeSet Attrs = F.getAttributes(), NewAttrs;
-
-    if (!CPU.empty())
-      NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
-                                       "target-cpu", CPU);
-
-    if (!Features.empty())
-      NewAttrs = NewAttrs.addAttribute(Ctx, AttributeSet::FunctionIndex,
-                                       "target-features", Features);
-
-    if (Options.NoFramePointerElimOverride || AlwaysRecordAttrs)
-      NewAttrs = NewAttrs.addAttribute(
-          Ctx, AttributeSet::FunctionIndex, "no-frame-pointer-elim",
-          Options.NoFramePointerElim ? "true" : "false");
-
-    // Let NewAttrs override Attrs.
-    NewAttrs = Attrs.addAttributes(Ctx, AttributeSet::FunctionIndex, NewAttrs);
-    F.setAttributes(NewAttrs);
-  }
-}

llvm/trunk/lib/ExecutionEngine/ExecutionEngineBindings.cpp

LLVMBool LLVMCreateMCJITCompilerForModule(
   // Defend against the user having an old version of the API by ensuring that
   // any fields they didn't see are cleared. We must defend against fields being
   // set to the bitwise equivalent of zero, and assume that this means "do the
   // default" as if that option hadn't been available.
   LLVMInitializeMCJITCompilerOptions(&options, sizeof(options));
   memcpy(&options, PassedOptions, SizeOfPassedOptions);
   
   TargetOptions targetOptions;
-  targetOptions.NoFramePointerElim = options.NoFramePointerElim;
   targetOptions.EnableFastISel = options.EnableFastISel;
   std::unique_ptr<Module> Mod(unwrap(M));
 
   if (Mod)
     // Set function attribute "no-frame-pointer-elim" based on
     // NoFramePointerElim.
-    setFunctionAttributes(/* CPU */ "", /* Features */ "", targetOptions, *Mod,
-                          /* AlwaysRecordAttrs */ true);
+    for (auto &F : *Mod) {
+      auto Attrs = F.getAttributes();
+      auto Value = options.NoFramePointerElim ? "true" : "false";
+      Attrs = Attrs.addAttribute(F.getContext(), AttributeSet::FunctionIndex,
+                                 "no-frame-pointer-elim", Value);
+      F.setAttributes(Attrs);
+    }
 
   std::string Error;
   EngineBuilder builder(std::move(Mod));
   builder.setEngineKind(EngineKind::JIT)
          .setErrorStr(&Error)
          .setOptLevel((CodeGenOpt::Level)options.OptLevel)
          .setCodeModel(unwrap(options.CodeModel))
          .setTargetOptions(targetOptions);

llvm/trunk/lib/ExecutionEngine/LLVMBuild.txt

 
 [common]
 subdirectories = Interpreter MCJIT RuntimeDyld IntelJITEvents OProfileJIT Orc
 
 [component_0]
 type = Library
 name = ExecutionEngine
 parent = Libraries
-required_libraries = CodeGen Core MC Object RuntimeDyld Support
+required_libraries = Core MC Object RuntimeDyld Support

llvm/trunk/tools/llc/llc.cpp

static int compileModule(char **argv, LLVMContext &Context) {
   if (DisableSimplifyLibCalls)
     TLII.disableAllFunctions();
   PM.add(new TargetLibraryInfoWrapperPass(TLII));
 
   // Add the target data from the target machine, if it exists, or the module.
   if (const DataLayout *DL = Target->getDataLayout())
     M->setDataLayout(*DL);
 
-  // Override function attributes based on CPUStr, FeaturesStr, and Options.
-  // Pass AlwaysRecordAttrs=false as we want to override an attribute only when
-  // the corresponding cl::opt has been provided on llc's command line.
-  setFunctionAttributes(CPUStr, FeaturesStr, Options, *M,
-                        /* AlwaysRecordAttrs */ false);
+  // Override function attributes based on CPUStr, FeaturesStr, and command line
+  // flags.
+  setFunctionAttributes(CPUStr, FeaturesStr, *M);
 
   if (RelaxAll.getNumOccurrences() > 0 &&
       FileType != TargetMachine::CGFT_ObjectFile)
     errs() << argv[0]
              << ": warning: ignoring -mc-relax-all because filetype != obj";
 
   {
     raw_pwrite_stream *OS = &Out->os();

llvm/trunk/tools/opt/opt.cpp

#endif
   if (ModuleTriple.getArch()) {
     CPUStr = getCPUStr();
     FeaturesStr = getFeaturesStr();
     Machine = GetTargetMachine(ModuleTriple, CPUStr, FeaturesStr, Options);
   }
 
   std::unique_ptr<TargetMachine> TM(Machine);
 
-  // Override function attributes based on CPUStr, FeaturesStr, and Options.
-  // Pass AlwaysRecordAttrs=false as we want to override an attribute only when
-  // the corresponding cl::opt has been provided on opt's command line.
-  setFunctionAttributes(CPUStr, FeaturesStr, Options, *M,
-                        /* AlwaysRecordAttrs */ false);
+  // Override function attributes based on CPUStr, FeaturesStr, and command line
+  // flags.
+  setFunctionAttributes(CPUStr, FeaturesStr, *M);
 
   // If the output is set to be emitted to standard out, and standard out is a
   // console, print out a warning message and refuse to do it.  We don't
   // impress anyone by spewing tons of binary goo to a terminal.
   if (!Force && !NoOutput && !AnalyzeOnly && !OutputAssembly)
     if (CheckBitcodeOutputToConsole(Out->os(), !Quiet))
       NoOutput = true;