misc clean up

Author: inbelic

clang/lib/Sema/SemaHLSL.cpp

@@ -1155,10 +1155,9 @@ bool SemaHLSL::handleRootSignatureElements(
     }
   }
 
-  // Sort as specified
-  auto ComparePairs = [](InfoPair A, InfoPair B) { return A.first < B.first; };
-
-  std::sort(InfoPairs.begin(), InfoPairs.end(), ComparePairs);
+  // 2. Sort with the RangeInfo <operator to prepare it for findOverlapping
+  std::sort(InfoPairs.begin(), InfoPairs.end(),
+            [](InfoPair A, InfoPair B) { return A.first < B.first; });
 
   llvm::SmallVector<RangeInfo> Infos;
   for (const InfoPair &Pair : InfoPairs)
@@ -1170,12 +1169,22 @@ bool SemaHLSL::handleRootSignatureElements(
                              const hlsl::RootSignatureElement *>;
   auto GetElemPair = [&Infos, &InfoPairs, &DuplicateCounter](
                          OverlappingRanges Overlap) -> ElemPair {
+    // Given we sorted the InfoPairs (and by implication) Infos, and,
+    // that Overlap.B is the item retrieved from the ResourceRange. Then it is
+    // guarenteed that Overlap.B <= Overlap.A.
+    //
+    // So we will find Overlap.B first and then continue to find Overlap.A
+    // after
     auto InfoB = std::lower_bound(Infos.begin(), Infos.end(), *Overlap.B);
     auto DistB = std::distance(Infos.begin(), InfoB);
     auto PairB = InfoPairs.begin();
     std::advance(PairB, DistB);
 
     auto InfoA = std::lower_bound(InfoB, Infos.end(), *Overlap.A);
+    // Similarily, from the property that we have sorted the RangeInfos,
+    // all duplicates will be processed one after the other. So
+    // DuplicateCounter can be re-used for each set of duplicates we
+    // encounter as we handle incoming errors
     DuplicateCounter = InfoA == InfoB ? DuplicateCounter + 1 : 0;
     auto DistA = std::distance(InfoB, InfoA) + DuplicateCounter;
     auto PairA = PairB;
@@ -1205,6 +1214,7 @@ bool SemaHLSL::handleRootSignatureElements(
     this->Diag(OElem->getLocation(), diag::note_hlsl_resource_range_here);
   };
 
+  // 3. Invoke find overlapping ranges
   llvm::SmallVector<OverlappingRanges> Overlaps =
       llvm::hlsl::rootsig::findOverlappingRanges(Infos);
   for (OverlappingRanges Overlap : Overlaps)

clang/test/SemaHLSL/RootSignature-resource-ranges-err.hlsl

@@ -107,32 +107,32 @@ void valid_root_signature_13() {}
 void bad_root_signature_14() {}
 
 #define DuplicatesRootSignature \
-  "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+  "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 
 // CHECK: [[@LINE-2]]:13: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-3]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+// CHECK-NEXT: [[@LINE-3]] |   "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 // CHECK-NEXT:             |             ^
 // CHECK: [[@LINE-5]]:4: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-6]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+// CHECK-NEXT: [[@LINE-6]] |    "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 // CHECK-NEXT:             |    ^
 // CHECK: [[@LINE-8]]:22: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-9]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+// CHECK-NEXT: [[@LINE-9]] |    "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 // CHECK-NEXT:             |                      ^
 // CHECK: [[@LINE-11]]:4: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-12]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+// CHECK-NEXT: [[@LINE-12]] |    "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 // CHECK-NEXT:              |    ^
-// CHECK: [[@LINE-14]]:31: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-15]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
-// CHECK-NEXT:              |                               ^
+// CHECK: [[@LINE-14]]:47: note: expanded from macro 'DuplicatesRootSignature'
+// CHECK-NEXT: [[@LINE-15]] |    "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
+// CHECK-NEXT:              |                                                ^
 // CHECK: [[@LINE-17]]:4: note: expanded from macro 'DuplicatesRootSignature'
-// CHECK-NEXT: [[@LINE-18]] |   "CBV(b0), CBV(b0), CBV(b0), CBV(b0)"
+// CHECK-NEXT: [[@LINE-18]] |    "CBV(b0), CBV(b0), CBV(b0), DescriptorTable(CBV(b0, numDescriptors = 2))"
 // CHECK-NEXT:              |    ^
 
 // expected-error@+6 {{resource ranges b[0;0] and b[0;0] overlap within space = 0 and visibility = All}}
 // expected-note@+5 {{overlapping resource range here}}
 // expected-error@+4 {{resource ranges b[0;0] and b[0;0] overlap within space = 0 and visibility = All}}
 // expected-note@+3 {{overlapping resource range here}}
-// expected-error@+2 {{resource ranges b[0;0] and b[0;0] overlap within space = 0 and visibility = All}}
+// expected-error@+2 {{resource ranges b[0;1] and b[0;0] overlap within space = 0 and visibility = All}}
 // expected-note@+1 {{overlapping resource range here}}
 [RootSignature(DuplicatesRootSignature)]
 void valid_root_signature_15() {}

llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h

@@ -137,8 +137,8 @@ struct OverlappingRanges {
 ///   - RangeInfo will retain the interval, ResourceClass, Space and Visibility
 ///   - It will also contain an index so that it can be associated to
 /// additional diagnostic information
-/// 2. Sort the RangeInfo's such that they are grouped together by
-///  ResourceClass and Space
+/// 2. The user is required to sort the RangeInfo's such that they are grouped
+/// together by ResourceClass and Space
 /// 3. Iterate through the collected RangeInfos by their groups
 ///   - For each group we will have a ResourceRange for each visibility
 ///   - As we iterate through we will:

llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp

@@ -245,7 +245,8 @@ std::optional<const RangeInfo *> ResourceRange::insert(const RangeInfo &Info) {
 
 llvm::SmallVector<OverlappingRanges>
 findOverlappingRanges(ArrayRef<RangeInfo> Infos) {
-  // 1. The user has provided the corresponding range information
+  // It is expected that Infos is filled with valid RangeInfos and that
+  // they are sorted with respect to the RangeInfo <operator
   llvm::SmallVector<OverlappingRanges> Overlaps;
   using GroupT = std::pair<dxil::ResourceClass, /*Space*/ uint32_t>;