[SYCL] Refactor of FPGA loop fusion function attributes

clang/include/clang/Sema/Sema.h

@@ -3471,8 +3471,6 @@ class Sema final {
   EnforceTCBLeafAttr *mergeEnforceTCBLeafAttr(Decl *D,
                                               const EnforceTCBLeafAttr &AL);
 
-  SYCLIntelLoopFuseAttr *
-  mergeSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E);
   void mergeDeclAttributes(NamedDecl *New, Decl *Old,
                            AvailabilityMergeKind AMK = AMK_Redeclaration);
   void MergeTypedefNameDecl(Scope *S, TypedefNameDecl *New,
@@ -10232,6 +10230,10 @@ class Sema final {
                                           Expr *E);
   SYCLIntelNoGlobalWorkOffsetAttr *MergeSYCLIntelNoGlobalWorkOffsetAttr(
       Decl *D, const SYCLIntelNoGlobalWorkOffsetAttr &A);
+  void AddSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI,
+                                Expr *E);
+  SYCLIntelLoopFuseAttr *
+  MergeSYCLIntelLoopFuseAttr(Decl *D, const SYCLIntelLoopFuseAttr &A);
 
   /// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
   void AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
@@ -10286,8 +10288,6 @@ class Sema final {
   /// addSYCLIntelPipeIOAttr - Adds a pipe I/O attribute to a particular
   /// declaration.
   void addSYCLIntelPipeIOAttr(Decl *D, const AttributeCommonInfo &CI, Expr *ID);
-  void addSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI,
-                                Expr *E);
 
   bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);
   bool checkAllowedSYCLInitializer(VarDecl *VD,

clang/lib/CodeGen/CodeGenFunction.cpp

@@ -983,10 +983,11 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
 
   if (getLangOpts().SYCLIsDevice && D) {
     if (const auto *A = D->getAttr<SYCLIntelLoopFuseAttr>()) {
-      Expr *E = A->getValue();
+      const auto *CE = cast<ConstantExpr>(A->getValue());
+      Optional<llvm::APSInt> ArgVal = CE->getResultAsAPSInt();
       llvm::Metadata *AttrMDArgs[] = {
-          llvm::ConstantAsMetadata::get(Builder.getInt32(
-              E->getIntegerConstantExpr(D->getASTContext())->getZExtValue())),
+          llvm::ConstantAsMetadata::get(
+              Builder.getInt32(ArgVal->getZExtValue())),
           llvm::ConstantAsMetadata::get(
               A->isIndependent() ? Builder.getInt32(1) : Builder.getInt32(0))};
       Fn->setMetadata("loop_fuse",

clang/lib/Sema/SemaDecl.cpp

@@ -2612,8 +2612,8 @@ static bool mergeDeclAttribute(Sema &S, NamedDecl *D,
     NewAttr = S.mergeImportModuleAttr(D, *IMA);
   else if (const auto *INA = dyn_cast<WebAssemblyImportNameAttr>(Attr))
     NewAttr = S.mergeImportNameAttr(D, *INA);
-  else if (const auto *LFA = dyn_cast<SYCLIntelLoopFuseAttr>(Attr))
-    NewAttr = S.mergeSYCLIntelLoopFuseAttr(D, *LFA, LFA->getValue());
+  else if (const auto *A = dyn_cast<SYCLIntelLoopFuseAttr>(Attr))
+    NewAttr = S.MergeSYCLIntelLoopFuseAttr(D, *A);
   else if (const auto *TCBA = dyn_cast<EnforceTCBAttr>(Attr))
     NewAttr = S.mergeEnforceTCBAttr(D, *TCBA);
   else if (const auto *TCBLA = dyn_cast<EnforceTCBLeafAttr>(Attr))

clang/lib/Sema/SemaDeclAttr.cpp

@@ -3453,87 +3453,93 @@ static void handleMaxGlobalWorkDimAttr(Sema &S, Decl *D, const ParsedAttr &A) {
   S.addIntelSingleArgAttr<SYCLIntelMaxGlobalWorkDimAttr>(D, A, E);
 }
 
-SYCLIntelLoopFuseAttr *
-Sema::mergeSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI,
-                                 Expr *E) {
+// Handles [[intel::loop_fuse]] and [[intel::loop_fuse_independent]].
+void Sema::AddSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI,
+                                    Expr *E) {
+  if (!E->isValueDependent()) {
+    // Validate that we have an integer constant expression and then store the
+    // converted constant expression into the semantic attribute so that we
+    // don't have to evaluate it again later.
+    llvm::APSInt ArgVal;
+    ExprResult Res = VerifyIntegerConstantExpression(E, &ArgVal);
+    if (Res.isInvalid())
+      return;
+    E = Res.get();
 
-  if (const auto ExistingAttr = D->getAttr<SYCLIntelLoopFuseAttr>()) {
+    // This attribute requires a non-negative value.
+    if (ArgVal < 0) {
+      Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
+          << CI << /*non-negative*/ 1;
+      return;
+    }
+    // Check to see if there's a duplicate attribute with different values
+    // already applied to the declaration.
+    if (const auto *DeclAttr = D->getAttr<SYCLIntelLoopFuseAttr>()) {
+      // If the other attribute argument is instantiation dependent, we won't
+      // have converted it to a constant expression yet and thus we test
+      // whether this is a null pointer.
+      const auto *DeclExpr = dyn_cast<ConstantExpr>(DeclAttr->getValue());
+      if (DeclExpr && ArgVal != DeclExpr->getResultAsAPSInt()) {
+        Diag(CI.getLoc(), diag::warn_duplicate_attribute) << CI;
+        Diag(DeclAttr->getLoc(), diag::note_previous_attribute);
+        return;
+      }
+      // [[intel::loop_fuse]] and [[intel::loop_fuse_independent]] are
+      // incompatible.
+      // FIXME: If additional spellings are provided for this attribute,
+      // this code will do the wrong thing.
+      if (DeclAttr->getAttributeSpellingListIndex() !=
+          CI.getAttributeSpellingListIndex()) {
+        Diag(CI.getLoc(), diag::err_attributes_are_not_compatible)
+            << CI << DeclAttr;
+        Diag(DeclAttr->getLocation(), diag::note_conflicting_attribute);
+        return;
+      }
+    }
+  }
+
+  D->addAttr(::new (Context) SYCLIntelLoopFuseAttr(Context, CI, E));
+}
+
+SYCLIntelLoopFuseAttr *
+Sema::MergeSYCLIntelLoopFuseAttr(Decl *D, const SYCLIntelLoopFuseAttr &A) {
+  // Check to see if there's a duplicate attribute with different values
+  // already applied to the declaration.
+  if (const auto *DeclAttr = D->getAttr<SYCLIntelLoopFuseAttr>()) {
+    const auto *DeclExpr = dyn_cast<ConstantExpr>(DeclAttr->getValue());
+    const auto *MergeExpr = dyn_cast<ConstantExpr>(A.getValue());
+    if (DeclExpr && MergeExpr &&
+        DeclExpr->getResultAsAPSInt() != MergeExpr->getResultAsAPSInt()) {
+      Diag(DeclAttr->getLoc(), diag::warn_duplicate_attribute) << &A;
+      Diag(A.getLoc(), diag::note_previous_attribute);
+      return nullptr;
+    }
     // [[intel::loop_fuse]] and [[intel::loop_fuse_independent]] are
     // incompatible.
     // FIXME: If additional spellings are provided for this attribute,
     // this code will do the wrong thing.
-    if (ExistingAttr->getAttributeSpellingListIndex() !=
-        CI.getAttributeSpellingListIndex()) {
-      Diag(CI.getLoc(), diag::err_attributes_are_not_compatible)
-          << CI << ExistingAttr;
-      Diag(ExistingAttr->getLocation(), diag::note_conflicting_attribute);
+    if (DeclAttr->getAttributeSpellingListIndex() !=
+        A.getAttributeSpellingListIndex()) {
+      Diag(A.getLoc(), diag::err_attributes_are_not_compatible)
+          << &A << DeclAttr;
+      Diag(DeclAttr->getLoc(), diag::note_conflicting_attribute);
       return nullptr;
     }
-
-    if (!E->isValueDependent()) {
-      Optional<llvm::APSInt> ArgVal = E->getIntegerConstantExpr(Context);
-      Optional<llvm::APSInt> ExistingArgVal =
-          ExistingAttr->getValue()->getIntegerConstantExpr(Context);
-
-      assert(ArgVal && ExistingArgVal &&
-             "Argument should be an integer constant expression");
-      // Compare attribute argument value and warn if there is a mismatch.
-      if (ArgVal->getExtValue() != ExistingArgVal->getExtValue())
-        Diag(ExistingAttr->getLoc(), diag::warn_duplicate_attribute)
-            << ExistingAttr;
-    }
-
-    // If there is no mismatch, silently ignore duplicate attribute.
-    return nullptr;
-  }
-  return ::new (Context) SYCLIntelLoopFuseAttr(Context, CI, E);
-}
-
-static bool checkSYCLIntelLoopFuseArgument(Sema &S,
-                                           const AttributeCommonInfo &CI,
-                                           Expr *E) {
-  // Dependent expressions are checked when instantiated.
-  if (E->isValueDependent())
-    return false;
-
-  Optional<llvm::APSInt> ArgVal = E->getIntegerConstantExpr(S.Context);
-  if (!ArgVal) {
-    S.Diag(E->getExprLoc(), diag::err_attribute_argument_type)
-        << CI << AANT_ArgumentIntegerConstant << E->getSourceRange();
-    return true;
-  }
-
-  if (!ArgVal->isNonNegative()) {
-    S.Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
-        << CI << /*non-negative*/ 1;
-    return true;
   }
 
-  return false;
+  return ::new (Context) SYCLIntelLoopFuseAttr(Context, A, A.getValue());
 }
 
-void Sema::addSYCLIntelLoopFuseAttr(Decl *D, const AttributeCommonInfo &CI,
-                                    Expr *E) {
-  assert(E && "argument has unexpected null value");
-
-  if (checkSYCLIntelLoopFuseArgument(*this, CI, E))
-    return;
-
-  SYCLIntelLoopFuseAttr *NewAttr = mergeSYCLIntelLoopFuseAttr(D, CI, E);
-
-  if (NewAttr)
-    D->addAttr(NewAttr);
-}
+static void handleSYCLIntelLoopFuseAttr(Sema &S, Decl *D, const ParsedAttr &A) {
+  S.CheckDeprecatedSYCLAttributeSpelling(A);
 
-// Handles [[intel::loop_fuse]] and [[intel::loop_fuse_independent]].
-static void handleLoopFuseAttr(Sema &S, Decl *D, const ParsedAttr &Attr) {
-  // Default argument value is set to 1.
-  Expr *E = Attr.isArgExpr(0)
-                ? Attr.getArgAsExpr(0)
+  // If no attribute argument is specified, set to default value '1'.
+  Expr *E = A.isArgExpr(0)
+                ? A.getArgAsExpr(0)
                 : IntegerLiteral::Create(S.Context, llvm::APInt(32, 1),
-                                         S.Context.IntTy, Attr.getLoc());
+                                         S.Context.IntTy, A.getLoc());
 
-  S.addSYCLIntelLoopFuseAttr(D, Attr, E);
+  S.AddSYCLIntelLoopFuseAttr(D, A, E);
 }
 
 static void handleVecTypeHint(Sema &S, Decl *D, const ParsedAttr &AL) {
@@ -9136,7 +9142,7 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
     handleUseStallEnableClustersAttr(S, D, AL);
     break;
   case ParsedAttr::AT_SYCLIntelLoopFuse:
-    handleLoopFuseAttr(S, D, AL);
+    handleSYCLIntelLoopFuseAttr(S, D, AL);
     break;
   case ParsedAttr::AT_VecTypeHint:
     handleVecTypeHint(S, D, AL);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -640,7 +640,7 @@ static void instantiateSYCLIntelLoopFuseAttr(
       S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   ExprResult Result = S.SubstExpr(Attr->getValue(), TemplateArgs);
   if (!Result.isInvalid())
-    S.addSYCLIntelLoopFuseAttr(New, *Attr, Result.getAs<Expr>());
+    S.AddSYCLIntelLoopFuseAttr(New, *Attr, Result.getAs<Expr>());
 }
 
 static void instantiateIntelReqdSubGroupSize(

clang/test/SemaSYCL/loop_fusion.cpp

@@ -1,63 +1,108 @@
 // RUN: %clang_cc1 -fsycl -fsycl-is-device -verify %s
 
+// Tests for incorrect argument values for Intel FPGA loop fusion function attributes
 [[intel::loop_fuse(5)]] int a; // expected-error{{'loop_fuse' attribute only applies to functions}}
 
-[[intel::loop_fuse("foo")]] void func() {} // expected-error{{'loop_fuse' attribute requires an integer constant}}
+[[intel::loop_fuse("foo")]] void func() {} // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'const char [4]'}}
 
 [[intel::loop_fuse(1048577)]] void func1() {}        // OK
 [[intel::loop_fuse_independent(-1)]] void func2() {} // expected-error{{'loop_fuse_independent' attribute requires a non-negative integral compile time constant expression}}
 
 [[intel::loop_fuse(0, 1)]] void func3() {}             // expected-error{{'loop_fuse' attribute takes no more than 1 argument}}
 [[intel::loop_fuse_independent(2, 3)]] void func4() {} // expected-error{{'loop_fuse_independent' attribute takes no more than 1 argument}}
 
+// Tests for Intel FPGA loop attributes duplication
 // No diagnostic is thrown since arguments match. Duplicate attribute is silently ignored.
 [[intel::loop_fuse]] [[intel::loop_fuse]] void func5() {}
 [[intel::loop_fuse_independent(10)]] [[intel::loop_fuse_independent(10)]] void func6() {}
 
-[[intel::loop_fuse]] [[intel::loop_fuse(10)]] void func7() {}                            // expected-warning {{attribute 'loop_fuse' is already applied with different arguments}}
-[[intel::loop_fuse_independent(5)]] [[intel::loop_fuse_independent(10)]] void func8() {} // expected-warning {{attribute 'loop_fuse_independent' is already applied with different arguments}}
+// Tests for merging of different argument values for Intel FPGA loop fusion function attributes
+[[intel::loop_fuse]]                     // expected-note {{previous attribute is here}}
+[[intel::loop_fuse(10)]] void func7() {} // expected-warning {{attribute 'loop_fuse' is already applied with different arguments}}
+
+[[intel::loop_fuse_independent(5)]]                  // expected-note {{previous attribute is here}}
+[[intel::loop_fuse_independent(10)]] void func8() {} // expected-warning {{attribute 'loop_fuse_independent' is already applied with different arguments}}
 
 [[intel::loop_fuse]] void func9();
-[[intel::loop_fuse]] void func9();
+[[intel::loop_fuse]] void func9(); // OK
 
 [[intel::loop_fuse_independent(10)]] void func10();
-[[intel::loop_fuse_independent(10)]] void func10();
+[[intel::loop_fuse_independent(10)]] void func10(); // OK
 
-[[intel::loop_fuse(1)]] void func11();
+[[intel::loop_fuse(1)]] void func11(); // expected-note {{previous attribute is here}}
 [[intel::loop_fuse(3)]] void func11(); // expected-warning {{attribute 'loop_fuse' is already applied with different arguments}}
 
-[[intel::loop_fuse_independent(1)]] void func12();
+[[intel::loop_fuse_independent(1)]] void func12(); // expected-note {{previous attribute is here}}
 [[intel::loop_fuse_independent(3)]] void func12(); // expected-warning {{attribute 'loop_fuse_independent' is already applied with different arguments}}
 
+[[intel::loop_fuse_independent]]
+[[intel::loop_fuse_independent]] void func13() {} // OK
+
+[[intel::loop_fuse_independent]]                      // expected-note {{previous attribute is here}}
+[[intel::loop_fuse_independent(10)]] void func14() {} // expected-warning {{attribute 'loop_fuse_independent' is already applied with different arguments}}
+
+// Tests for Intel FPGA loop fusion function attributes compatibility
 // expected-error@+2 {{'loop_fuse_independent' and 'loop_fuse' attributes are not compatible}}
 // expected-note@+1 {{conflicting attribute is here}}
-[[intel::loop_fuse]] [[intel::loop_fuse_independent]] void func13();
+[[intel::loop_fuse]] [[intel::loop_fuse_independent]] void func15();
 
 // expected-error@+2 {{'loop_fuse' and 'loop_fuse_independent' attributes are not compatible}}
 // expected-note@+1 {{conflicting attribute is here}}
-[[intel::loop_fuse_independent]] [[intel::loop_fuse]] void func14();
+[[intel::loop_fuse_independent]] [[intel::loop_fuse]] void func16();
 
 // expected-error@+2 {{'loop_fuse' and 'loop_fuse_independent' attributes are not compatible}}
 // expected-note@+2 {{conflicting attribute is here}}
-[[intel::loop_fuse]] void func15();
-[[intel::loop_fuse_independent]] void func15();
+[[intel::loop_fuse]] void func17();
+[[intel::loop_fuse_independent]] void func17();
 
 // expected-error@+2 {{'loop_fuse_independent' and 'loop_fuse' attributes are not compatible}}
 // expected-note@+2 {{conflicting attribute is here}}
-[[intel::loop_fuse_independent]] void func16();
-[[intel::loop_fuse]] void func16();
+[[intel::loop_fuse_independent]] void func18();
+[[intel::loop_fuse]] void func18();
 
+// Tests that check template parameter support for Intel FPGA loop fusion function attributes
 template <int N>
-[[intel::loop_fuse(N)]] void func17(); // expected-error{{'loop_fuse' attribute requires a non-negative integral compile time constant expression}}
+[[intel::loop_fuse(N)]] void func19(); // expected-error{{'loop_fuse' attribute requires a non-negative integral compile time constant expression}}
 
-template <typename Ty>
-[[intel::loop_fuse(Ty{})]] void func18() {} // expected-error{{'loop_fuse' attribute requires an integer constant}}
+template <int size>
+[[intel::loop_fuse(12)]] void func20();     // expected-note {{previous attribute is here}}
+template <int size>
+[[intel::loop_fuse(size)]] void func20() {} // expected-warning {{attribute 'loop_fuse' is already applied with different arguments}}
+
+template <int size>
+[[intel::loop_fuse_independent(5)]] void func21();      // expected-note {{previous attribute is here}}
+template <int size>
+[[intel::loop_fuse_independent(size)]] void func21() {} // expected-warning {{attribute 'loop_fuse_independent' is already applied with different arguments}}
 
 void checkTemplates() {
-  func17<-1>();    // expected-note{{in instantiation of}}
-  func17<0>();     // OK
-  func18<float>(); // expected-note{{in instantiation of}}
+  func19<-1>(); // expected-note{{in instantiation of}}
+  func19<0>();  // OK
+  func20<20>(); // expected-note {{in instantiation of function template specialization 'func20<20>' requested here}}
+  func21<14>(); // expected-note {{in instantiation of function template specialization 'func21<14>' requested here}}
 }
 
+// Test that checks expression is not a constant expression.
+// expected-note@+1{{declared here}}
 int baz();
-[[intel::loop_fuse(baz())]] void func19(); // expected-error{{'loop_fuse' attribute requires an integer constant}}
+// expected-error@+2{{expression is not an integral constant expression}}
+// expected-note@+1{{non-constexpr function 'baz' cannot be used in a constant expression}}
+[[intel::loop_fuse(baz() + 1)]] void func22();
+
+// Test that checks expression is a constant expression.
+constexpr int bar() { return 0; }
+[[intel::loop_fuse(bar() + 2)]] void func23(); // OK
+
+// Test that checks wrong function template instantiation and ensures that the type
+// is checked properly when instantiating from the template definition.
+template <typename Ty>
+// expected-error@+2 {{integral constant expression must have integral or unscoped enumeration type, not 'S'}}
+// expected-error@+1 {{integral constant expression must have integral or unscoped enumeration type, not 'float'}}
+[[intel::loop_fuse(Ty{})]] void func24() {}
+
+struct S {};
+void test() {
+  //expected-note@+1{{in instantiation of function template specialization 'func24<S>' requested here}}
+  func24<S>();
+  //expected-note@+1{{in instantiation of function template specialization 'func24<float>' requested here}}
+  func24<float>();
+}