[Clang] Copy strictfp attribute from pattern to instantiation

[sepavloff]

Summary

If a template function contained a pragma that made it strictfp, code
generation for such function crashed, because the instantiation did not
have strictfp attribute. As a solution this attribute is copied from the
template to instantiation.

[efriedma]

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

[sepavloff]

In D143919#4123616, @efriedma wrote:

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

Could you please point me out to this code? I didn't find it.

[rjmccall]

In D143919#4123712, @sepavloff wrote:

In D143919#4123616, @efriedma wrote:

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

Could you please point me out to this code? I didn't find it.

It's the call to InstantiateAttrs in InitFunctionInstantiation. InstantiateAttrs hard-codes a list of attributes to clone, which seems like a bad design, but it's easy enough to add your attribute as another case to that list.

[sepavloff]

Changed the way to copy strictfp attribute

[sepavloff]

In D143919#4123922, @rjmccall wrote:

In D143919#4123712, @sepavloff wrote:

In D143919#4123616, @efriedma wrote:

We have code somewhere to generically copy attributes from function templates to instantiations, right? Why do we need to explicitly copy StrictFPAttr in particular, separate from that?

Could you please point me out to this code? I didn't find it.

It's the call to InstantiateAttrs in InitFunctionInstantiation. InstantiateAttrs hard-codes a list of attributes to clone, which seems like a bad design, but it's easy enough to add your attribute as another case to that list.

Thanks!
InstantiateAttrs is updated accordingly. It solves the problem but not for late-parsed templates. Strictfp is evaluated based on function body, but when a declaration for late-parsed template is instantiated, the body is absent yet. A new method is added to Sema to make attribute update when template body is available.

[efriedma]

If the "strictfp" attribute is based on the contents of the function body, should we recompute it when the function is instantiated, as opposed to copying it? For example, say you have a pragma inside an "if constexpr"; should that set the strictfp attribute if it's discarded?

Otherwise, I guess updateAttrsForLateParsedTemplate makes sense.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

817

Is this necessary? The non-delayed-parsed case seems to work correctly on trunk without any changes, so I suspect the autogenerated sema::instantiateTemplateAttribute is doing the right thing.

[sepavloff]

In D143919#4130178, @efriedma wrote:

If the "strictfp" attribute is based on the contents of the function body, should we recompute it when the function is instantiated, as opposed to copying it? For example, say you have a pragma inside an "if constexpr"; should that set the strictfp attribute if it's discarded?

Otherwise, I guess updateAttrsForLateParsedTemplate makes sense.

Recomputing is certainly a better solution, as it would provide actual properties. It however should be applied to all instantiations, not only late parsed. With this change the code:

template <typename T, bool F>
T templ_01(T x, T y) {
  if constexpr (F) {
    #pragma STDC FENV_ACCESS ON
    return x - y;
  } else
  return x + y;
}

float func_02(float x, float y) {
  return templ_01<float, true>(x, y);
}

float func_03(float x, float y) {
  return templ_01<float, false>(x, y);
}

compiled without late parsing produces func_03 with strictfp attribute. However constarined intrinsics in it are semantically equivalent to non-constrained operations. Adding unneeded strictfp attribute is not an error.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

817

Yes, it is necessary. Without it the code from the added test crashes, as constrained intrinsic is used in a function without strictfp attribute.

[efriedma]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

817

https://godbolt.org/z/MvPrejY75 indicates this is only an issue with -fdelayed-template-parsing; am I missing something?

[sepavloff]

Remove copying StrictFPAttr from InstantiateAttrs

[sepavloff]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

817

Indeed. I used Windows where this flag is on by default. Thank you for the catch.
Removed.

[efriedma]

LGTM

[sepavloff]

Thanks!

clang/include/clang/Sema/Sema.h

struct LateInstantiatedAttribute {
     { }
   };
   typedef SmallVector<LateInstantiatedAttribute, 16> LateInstantiatedAttrVec;
 
   void InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
                         const Decl *Pattern, Decl *Inst,
                         LateInstantiatedAttrVec *LateAttrs = nullptr,
                         LocalInstantiationScope *OuterMostScope = nullptr);
+  void updateAttrsForLateParsedTemplate(const Decl *Pattern, Decl *Inst);
 
   void
   InstantiateAttrsForDecl(const MultiLevelTemplateArgumentList &TemplateArgs,
                           const Decl *Pattern, Decl *Inst,
                           LateInstantiatedAttrVec *LateAttrs = nullptr,
                           LocalInstantiationScope *OuterMostScope = nullptr);
 
   void InstantiateDefaultCtorDefaultArgs(CXXConstructorDecl *Ctor);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

for (const auto *TmplAttr : Tmpl->attrs()) {
     if (auto *A = dyn_cast<SYCLKernelAttr>(TmplAttr)) {
       instantiateDependentSYCLKernelAttr(*this, TemplateArgs, *A, New);
       continue;
     }
 
     assert(!TmplAttr->isPackExpansion());
     if (TmplAttr->isLateParsed() && LateAttrs) {
       // Late parsed attributes must be instantiated and attached after the
       // enclosing class has been instantiated.  See Sema::InstantiateClass.
       LocalInstantiationScope *Saved = nullptr;
       if (CurrentInstantiationScope)
         Saved = CurrentInstantiationScope->cloneScopes(OuterMostScope);
       LateAttrs->push_back(LateInstantiatedAttribute(TmplAttr, Saved, New));
     } else {
       // Allow 'this' within late-parsed attributes.
       auto *ND = cast<NamedDecl>(New);
       auto *ThisContext = dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext());
       CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
                                  ND->isCXXInstanceMember());
 
       Attr *NewAttr = sema::instantiateTemplateAttribute(TmplAttr, Context,
                                                          *this, TemplateArgs);
       if (NewAttr && isRelevantAttr(*this, New, TmplAttr))
         New->addAttr(NewAttr);
     }
   }
 }
 
+/// Update instantiation attributes after template was late parsed.
+///
+/// Some attributes are evaluated based on the body of template. If it is
+/// late parsed, such attributes cannot be evaluated when declaration is
+/// instantiated. This function is used to update instantiation attributes when
+/// template definition is ready.
+void Sema::updateAttrsForLateParsedTemplate(const Decl *Pattern, Decl *Inst) {
+  for (const auto *Attr : Pattern->attrs()) {
+    if (auto *A = dyn_cast<StrictFPAttr>(Attr)) {
+      if (!Inst->hasAttr<StrictFPAttr>())
+        Inst->addAttr(A->clone(getASTContext()));
+      continue;
+    }
+  }
+}
+
 /// In the MS ABI, we need to instantiate default arguments of dllexported
 /// default constructors along with the constructor definition. This allows IR
 /// gen to emit a constructor closure which calls the default constructor with
 /// its default arguments.
 void Sema::InstantiateDefaultCtorDefaultArgs(CXXConstructorDecl *Ctor) {
   assert(Context.getTargetInfo().getCXXABI().isMicrosoft() &&
          Ctor->isDefaultConstructor());
   unsigned NumParams = Ctor->getNumParams();
if (!Pattern && PatternDecl->isLateTemplateParsed() &&
     if (PatternDecl->isFromASTFile())
       ExternalSource->ReadLateParsedTemplates(LateParsedTemplateMap);
 
     auto LPTIter = LateParsedTemplateMap.find(PatternDecl);
     assert(LPTIter != LateParsedTemplateMap.end() &&
            "missing LateParsedTemplate");
     LateTemplateParser(OpaqueParser, *LPTIter->second);
     Pattern = PatternDecl->getBody(PatternDecl);
+    updateAttrsForLateParsedTemplate(PatternDecl, Function);
   }
 
   // Note, we should never try to instantiate a deleted function template.
   assert((Pattern || PatternDecl->isDefaulted() ||
           PatternDecl->hasSkippedBody()) &&
          "unexpected kind of function template definition");
 
   // C++1y [temp.explicit]p10:

clang/test/CodeGen/fp-template.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -fdelayed-template-parsing -o - %s | FileCheck %s
+
+template <typename T>
+T templ_01(T x, T y) {
+#pragma STDC FENV_ACCESS ON
+  return x + y;
+}
+
+float func_01(float x, float y) {
+  return templ_01(x, y);
+}
+
+// CHECK-LABEL: define {{.*}} @_Z8templ_01IfET_S0_S0_
+// CHECK-SAME:  (float noundef %{{.*}}, float noundef %{{.*}}) #[[ATTR01:[0-9]+]]{{.*}} {
+// CHECK:       call float @llvm.experimental.constrained.fadd.f32
+
+// CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp