- add sema checks for loop unroll, switch.

- Remove need for custom parsing by adding default val
- Add nested loop tests

Author: farzonl

clang/include/clang/Basic/Attr.td

@@ -4120,12 +4120,11 @@ def HLSLLoopHint: StmtAttr {
   /// [unroll(directive)]
   /// [loop]
   let Spellings = [Microsoft<"unroll">, Microsoft<"loop">];
-  let Args = [UnsignedArgument<"directive">];
+  let Args = [UnsignedArgument<"directive", /*opt*/1>];
   let Subjects = SubjectList<[ForStmt, WhileStmt, DoStmt],
                               ErrorDiag, "'for', 'while', and 'do' statements">;
   let LangOpts = [HLSL];
   let Documentation = [HLSLLoopHintDocs, HLSLUnrollHintDocs];
-  let HasCustomParsing = 1;
 }
 
 def CapturedRecord : InheritableAttr {

clang/lib/Sema/SemaStmtAttr.cpp

@@ -587,27 +587,34 @@ static Attr *handleOpenCLUnrollHint(Sema &S, Stmt *St, const ParsedAttr &A,
 
 static Attr *handleHLSLLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
                                     SourceRange Range) {
+
+  if (A.getSemanticSpelling() == HLSLLoopHintAttr::Spelling::Microsoft_loop &&
+      !A.checkAtMostNumArgs(S, 0))
+    return nullptr;
+
   unsigned UnrollFactor = 0;
   if (A.getNumArgs() == 1) {
-    Expr *E = A.getArgAsExpr(0);
-    std::optional<llvm::APSInt> ArgVal;
 
-    if (!(ArgVal = E->getIntegerConstantExpr(S.Context))) {
+    if (A.isArgIdent(0)) {
       S.Diag(A.getLoc(), diag::err_attribute_argument_type)
-          << A << AANT_ArgumentIntegerConstant << E->getSourceRange();
+          << A << AANT_ArgumentIntegerConstant << A.getRange();
       return nullptr;
     }
 
-    int Val = ArgVal->getSExtValue();
-    if (Val <= 0) {
-      S.Diag(A.getRange().getBegin(),
-             diag::err_attribute_requires_positive_integer)
-          << A << /* positive */ 0;
+    Expr *E = A.getArgAsExpr(0);
+
+    if (S.CheckLoopHintExpr(E, St->getBeginLoc(),
+                            /*AllowZero=*/false))
       return nullptr;
-    }
+
+    std::optional<llvm::APSInt> ArgVal = E->getIntegerConstantExpr(S.Context);
+    // CheckLoopHintExpr handles non int const cases
+    assert(ArgVal != std::nullopt && "ArgVal should be an integer constant.");
+    int Val = ArgVal->getSExtValue();
+    // CheckLoopHintExpr handles negative and zero cases
+    assert(Val > 0 && "Val should be a positive integer greater than zero.");
     UnrollFactor = static_cast<unsigned>(Val);
   }
-
   return ::new (S.Context) HLSLLoopHintAttr(S.Context, A, UnrollFactor);
 }
 

clang/test/CodeGenHLSL/loops/unroll.hlsl

@@ -26,6 +26,34 @@ void for_enable()
 // CHECK: br label %{{.*}}, !llvm.loop ![[FOR_ENABLE:.*]]
 }
 
+void for_nested_one_unroll_enable()
+{
+// CHECK-LABEL: for_nested_one_unroll_enable
+    int s = 0;
+    [unroll]
+    for( int i = 0; i < 1000; ++i) {
+        for( int j = 0; j < 10; ++j)
+            s += i + j;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_NESTED_ENABLE:.*]]
+// CHECK-NOT: br label %{{.*}}, !llvm.loop ![[FOR_NESTED_1_ENABLE:.*]]
+}
+
+void for_nested_two_unroll_enable()
+{
+// CHECK-LABEL: for_nested_two_unroll_enable
+    int s = 0;
+    [unroll]
+    for( int i = 0; i < 1000; ++i) {
+        [unroll]
+        for( int j = 0; j < 10; ++j)
+            s += i + j;
+    }
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_NESTED2_ENABLE:.*]]
+// CHECK: br label %{{.*}}, !llvm.loop ![[FOR_NESTED2_1_ENABLE:.*]]
+}
+
+
 /*** while ***/
 void while_count()
 {
@@ -89,6 +117,9 @@ void do_enable()
 // CHECK: ![[DISABLE]]       =  !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[FOR_ENABLE]]      =  distinct !{![[FOR_ENABLE]],  ![[ENABLE:.*]]}
 // CHECK: ![[ENABLE]]          =  !{!"llvm.loop.unroll.enable"}
+// CHECK: ![[FOR_NESTED_ENABLE]] =  distinct !{![[FOR_NESTED_ENABLE]], ![[ENABLE]]}
+// CHECK: ![[FOR_NESTED2_ENABLE]] =  distinct !{![[FOR_NESTED2_ENABLE]], ![[ENABLE]]}
+// CHECK: ![[FOR_NESTED2_1_ENABLE]] =  distinct !{![[FOR_NESTED2_1_ENABLE]], ![[ENABLE]]}
 // CHECK: ![[WHILE_DISTINCT]]   =  distinct !{![[WHILE_DISTINCT]],    ![[WHILE_COUNT:.*]]}
 // CHECK: ![[WHILE_COUNT]]         =  !{!"llvm.loop.unroll.count", i32 4}
 // CHECK: ![[WHILE_DISABLE]] =  distinct !{![[WHILE_DISABLE]],  ![[DISABLE]]}

clang/test/SemaHLSL/Loops/unroll.hlsl

@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -O0 -finclude-default-header -fsyntax-only -triple dxil-pc-shadermodel6.6-library %s -verify
+void unroll_no_vars() {
+  int I = 3;
+  [unroll(I)]  // expected-error {{'unroll' attribute requires an integer constant}}
+  while (I--);
+}
+
+void unroll_arg_count() {
+   [unroll(2,4)] // expected-error {{'unroll' attribute takes one argument}}
+  for(int i=0; i<100; i++);
+}
+
+void loop_arg_count() {
+   [loop(2)] // expected-error {{'loop' attribute takes no more than 0 argument}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_no_negative() {
+  [unroll(-1)] // expected-error {{invalid value '-1'; must be positive}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_no_zero() {
+  [unroll(0)] // expected-error {{invalid value '0'; must be positive}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_no_float() {
+  [unroll(2.1)] // expected-error {{invalid argument of type 'float'; expected an integer type}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_no_bool_false() {
+  [unroll(false)] // expected-error {{invalid argument of type 'bool'; expected an integer type}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_no_bool_true() {
+  [unroll(true)] // expected-error {{invalid argument of type 'bool'; expected an integer type}}
+  for(int i=0; i<100; i++);
+}
+
+void unroll_loop_enforcement() {
+  int x[10];
+  [unroll(4)] // expected-error {{'unroll' attribute only applies to 'for', 'while', and 'do' statements}}
+  if (x[0])
+    x[0] = 15;
+}