[RFC] Storing default function attributes on the module

As we encode more CodeGen and target-specific options in bitcode to
support LTO, we risk crippling `llc` as a debugging tool. In
particular, `llc` command-line options are generally ignored when a
function has an attribute set explicitly, but the plan of record is for
`clang` to explicitly encode all (or most) CodeGen options -- even the
target defaults.

Changing `clang` to store target defaults on the module will allow us to
continue to override them when running `llc`. The right precedence
would be:

  1. Explicit attributes set on the function.
  2. `llc` command-line options.
  3. Default function attributes stored on the module.

(Outside of `llc`, skip step 2.)

In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
defaults should be pushed down as explicit function attributes.

Default function-level attributes

0001-IR-Canonicalize-access-to-function-attributes-N-llvm.patch (61.7 KB)

0002-IR-Add-default-function-attributes-to-Module-llvm.patch (30.6 KB)

0003-CodeGen-Set-module-wide-attributes-on-the-modu-clang.patch (4.11 KB)

Are llc command line options all that critical? It’s not that hard to edit the attributes directly or remove them with sed.

The less codegen depends on llc command line flags, the better, IMO.

+grosbach

Are llc command line options all that critical? It's not that hard to edit the attributes directly or remove them with sed.

Maybe Jim can speak to this one better than I can, but the workflow
I've heard concerns about is:

  - Got a codegen bug (PR or whatever).
  - Want to fiddle with codegen options in `llc`, to see which ones
    affect the bug and which don't.
  - Don't want command-line options to influence attributes that
    were specified explicitly.
  - Obviously want to influence the others.

Sure, `sed` could do this, but it's manual and fairly error-prone,
and would have a pretty tough time figuring out which attributes
are there because they're target defaults vs. specified in the
source.

The less codegen depends on llc command line flags, the better, IMO.

This doesn't make sense to me. The only command-line flags in `llc`
are codegen options... so we remove all `llc` flags?

I'm not suggesting we push more command-line flags through CodeGen;
I just don't want `llc` to *break*. (IMO, `llc` could/should just
modify the module-level defaults I've added here, but that's not
part of this proposal since there seem to be a ton of weird issues
with command-line options and I don't really want to get involved.
Just looking to maintain current functionality.)

+grosbach

Are llc command line options all that critical? It's not that hard to edit the attributes directly or remove them with sed.

Maybe Jim can speak to this one better than I can, but the workflow
I've heard concerns about is:

- Got a codegen bug (PR or whatever).
- Want to fiddle with codegen options in `llc`, to see which ones
   affect the bug and which don't.
- Don't want command-line options to influence attributes that
   were specified explicitly.
- Obviously want to influence the others.

Sure, `sed` could do this, but it's manual and fairly error-prone,
and would have a pretty tough time figuring out which attributes
are there because they're target defaults vs. specified in the
source.

Yep. Duncan summarized it nicely. Breaking llc’s ability to use these options to debug problems will be a *very* big usability loss for LLVM backend devs.

Hi Duncan

The first patch is general goodness and I think should be committed now.

The other 2 LGTM. Unless anyone fundamentally objects to module attributes, or has feedback on the patches themselves, then please commit. I didn’t see any problems with them.

Thanks,
Pete

Hi Duncan

The first patch is general goodness and I think should be committed now.

That's what I thought.

I'll start committing.

The other 2 LGTM. Unless anyone fundamentally objects to module attributes, or has feedback on the patches themselves, then please commit. I didn’t see any problems with them.

Cool. I figure I'll sit on them at least the next weekly mail goes
out, just in case someone finds a problem with the direction.

Hi Duncan

The first patch is general goodness and I think should be committed now.

The other 2 LGTM. Unless anyone fundamentally objects to module
attributes, or has feedback on the patches themselves, then please commit.
I didn’t see any problems with them.

I'm wondering if we could wire the LLC options to just change what Duncan's
OP called "default module options"? If we have a way to transport these
settings to the backend in the module as data, having a path in the code
seems redundant; better to canonicalize on always getting this information
to the backend through the module. As a bonus, that helps with Reid's
concern too, I think.

-- Sean Silva

As we encode more CodeGen and target-specific options in bitcode to
support LTO, we risk crippling `llc` as a debugging tool. In
particular, `llc` command-line options are generally ignored when a
function has an attribute set explicitly, but the plan of record is for
`clang` to explicitly encode all (or most) CodeGen options -- even the
target defaults.

Changing `clang` to store target defaults on the module will allow us to
continue to override them when running `llc`. The right precedence
would be:

  1. Explicit attributes set on the function.
  2. `llc` command-line options.
  3. Default function attributes stored on the module.

(Outside of `llc`, skip step 2.)

In `lib/Linker` (i.e., `llvm-lto`, `llvm-link`, `libLTO.dylib`),
defaults should be pushed down as explicit function attributes.

Default function-level attributes

I've attached patches with a reference implementation.

  - 0001: Canonicalize access to function attributes to use
    `getFnAttribute()` and `hasFnAttribute()`. (This seems like a nice
    cleanup regardless?)
  - 0002: Add the feature.

Random comment, but this patch changes the IR without any updates to
LangRef (or its sub-pages).

-- Sean Silva

Sounds good to me. It would be really nice to have a single place to look to see the default options, and you’ll see them every time you dump the module too which is a bonus.

Pete