Page MenuHomePhabricator

[PATCH] Stop resetting NoFramePointerElim in TargetMachine::resetTargetOptions

Authored by ahatanak on May 18 2015, 11:44 AM.



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).

Diff Detail


Event Timeline

ahatanak updated this revision to Diff 25991.May 18 2015, 11:44 AM
ahatanak retitled this revision from to [PATCH] Stop resetting NoFramePointerElim in TargetMachine::resetTargetOptions.
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added reviewers: echristo, dexonsmith.
ahatanak added a subscriber: Unknown Object (MLST).
echristo accepted this revision.May 18 2015, 2:33 PM
echristo edited edge metadata.

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.


This revision is now accepted and ready to land.May 18 2015, 2:33 PM

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.

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.


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 updated this revision to Diff 26197.May 20 2015, 5:26 PM
ahatanak edited edge metadata.

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?

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!


233 ↗(On Diff #26197)

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

185 ↗(On Diff #26197)

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

1 ↗(On Diff #26197)

Should probably make this commit separately?

308 ↗(On Diff #26197)

Comment the false argument.

400 ↗(On Diff #26197)

Comment the false argument.

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?

185 ↗(On Diff #26197)

OK, will fix.

1 ↗(On Diff #26197)

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

308 ↗(On Diff #26197)

Will fix.

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 updated this revision to Diff 26528.May 26 2015, 12:34 PM

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.

This works for me, thanks.


This revision was automatically updated to reflect the committed changes.