[PATCH] D126984: [clang] Add initial support for gcc's optimize function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126984#3574077 <https://reviews.llvm.org/D126984#3574077>, @aeubanks wrote:

> In D126984#3574046 <https://reviews.llvm.org/D126984#3574046>, @aaron.ballman wrote:
>
>> In D126984#3571573 <https://reviews.llvm.org/D126984#3571573>, @aeubanks wrote:
>>
>>> IIRC in the past there was a strong preference to not have the pass manager support this sort of thing
>>> if you want to support this, there should be an RFC for how the optimization part of this will work as it may require invasive changes to the LLVM pass manager
>>>
>>> (if this is purely a clang frontend thing then ignore me)
>>
>> Hmm, does the pass manager have to support anything here? The only Clang codegen changes are for emitting IR attributes that we already emitted based on command line flags/other attributes, so I had the impression this would not be invasive for the backend at all.
>
> if we're allowing individual functions to specify that they want the `-O1` pipeline when everything else in the module should be compiled with `-O2`, that's a huge change in the pass manager. but perhaps I'm misunderstanding the point of this patch

I guess I'm not seeing what burden is being added here (you may still be correct though!)

The codegen changes basically boil down to:

  if (!HasOptnone) {
    if (CodeGenOpts.OptimizeSize || HasOptsize) // This was updated for || HasOptsize
      FuncAttrs.addAttribute(llvm::Attribute::OptimizeForSize);
    if (CodeGenOpts.OptimizeSize == 2)
      FuncAttrs.addAttribute(llvm::Attribute::MinSize);
  }

and

  bool HasOptimizeAttrO0 = false;                                     // NEW
  if (const auto *OA = D->getAttr<OptimizeAttr>())
    HasOptimizeAttrO0 = OA->getOptLevel() == OptimizeAttr::O0;
  
  // Add optnone, but do so only if the function isn't always_inline.
  if ((ShouldAddOptNone || D->hasAttr<OptimizeNoneAttr>() ||
       HasOptimizeAttrO0) &&                                          // NEW
      !F->hasFnAttribute(llvm::Attribute::AlwaysInline)) {
    B.addAttribute(llvm::Attribute::OptimizeNone);

For my own education, are you saying that these changes are specifying the entire -O<N> pipeline?

The patch looks for `O0` in the attribute, but the other O<N> values are noops. We do some mapping though:

  OptimizeAttr::OptLevelKind Kind = OA->getOptLevel();
  HasOptnone = HasOptnone || (Kind == OptimizeAttr::O0);
  HasOptsize = Kind == OptimizeAttr::Os || Kind == OptimizeAttr::Og ||
               Kind == OptimizeAttr::Ofast || Kind == OptimizeAttr::Oz;

but I'm failing to understand why this would be a concern for the backend given that we already support setting the LLVM IR flags based on other attributes. e.g., why is `[[clang::optnone]]` okay but `[[gnu::optimize("O0")]]` a problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126984