[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

Thu Jun 2 09:47:28 PDT 2022

rnk added a comment.

In D125723#3553664 <https://reviews.llvm.org/D125723#3553664>, @aaron.ballman wrote:

> In D125723#3550956 <https://reviews.llvm.org/D125723#3550956>, @steplong wrote:
>
>> Appreciate the help! It's not clear to me how to go from the strings "Os", "foo1", "foo2", "foo3" to adding "-Os -ffoo1 -ffoo2 -ffoo3" to the compilation line for that function
>
> I may be misunderstanding the issue here, but what I had envisioned was that the `OptimizeAttr` would take either a string or integer argument (I don't know how easy that will be to model in Attr.td, FWIW; it may require adding a new kind of `Argument` for attributes) and then add a "fake" `Argument` or use the `AdditionalMembers` field to add a member that stores the converted "value" of that string or integer in whatever form makes the most sense for the semantics (I was envisioning an enumeration for the various kinds of optimization options, but maybe that doesn't work for the -f arguments?). Then, when doing CodeGen, you can look at the function to see if it has an `OptimizeAttr`, and if it does, use the fake/additional member to determine what information to lower to IR (or not lower, as the case may be). Does that help get you unstuck somewhat?
>
> Btw, one possibility would be to not support any -f flags initially and only support optimization levels, if that's easier. We can add support for individual flags at a later step but still get utility out of the attribute this way.

I would really like to limit the scope here to foreclose any future possibility of sending attribute strings through the command line parser. That would expose a big, general API surface, with lots of possibilities for confusing interactions with other features.

I think it will be cleaner to keep the attributes as close as possible to simple booleans, which then correspond relatively directly to the LLVM IR `optnone`, `optsize`, etc attributes.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125723/new/

https://reviews.llvm.org/D125723