[PATCH] D33352: [globalisel][tablegen] Fix incorrect inclusion of VS_32/VS_64 in [VS]GPRRegBank

[Daniel Sanders via Phabricator via llvm-commits]

dsanders created this revision.
Herald added subscribers: tpr, igorb, kristof.beyls, rengolin, aemerson.

Re-worked the code that adds register classes to register banks to fix a bug
that occurs when subregister indices are re-used for distinct register classes.
This occurred on AMDGPU and caused VS_32/VS_64 to be incorrectly added to both
VGPRRegBank and SGPRRegBank.

Previously the code would inspect a register class (X) that is known to be in
the register bank and check whether it implied that another register bank (Y)
should also be a member. This code would consider Y to be covered if every
register was accessible by a subregister index that was used by X to access
subregisters. However, it did not check whether the subregisters of X were
present in Y.

This worked fine for AArch64, ARM, and X86 but on AMDGPU it was possible for
the subregisters of X to be missing from Y due to the re-use of subregister
indices.

Now the code checks that every register in Y is either in X or is a
subregister of a register in X.

This has also has a side-effect on X86. It now notices that GR64_TCW64
should be in GPRRegBank.


https://reviews.llvm.org/D33352

Files:
  utils/TableGen/RegisterBankEmitter.cpp


Index: utils/TableGen/RegisterBankEmitter.cpp
===================================================================
--- utils/TableGen/RegisterBankEmitter.cpp
+++ utils/TableGen/RegisterBankEmitter.cpp
@@ -180,30 +180,43 @@
   VisitFn(RC, Kind.str());
 
   for (const auto &PossibleSubclass : RegisterClassHierarchy.getRegClasses()) {
+    if (RC == &PossibleSubclass)
+      continue;
+
     std::string TmpKind =
-        (Twine(Kind) + " (" + PossibleSubclass.getName() + ")").str();
+        (Twine(Kind) + " (then, while considering possible-subclass=" +
+         PossibleSubclass.getName())
+            .str();
 
     // Visit each subclass of an explicitly named class.
-    if (RC != &PossibleSubclass && RC->hasSubClass(&PossibleSubclass))
+    if (RC->hasSubClass(&PossibleSubclass))
       visitRegisterBankClasses(RegisterClassHierarchy, &PossibleSubclass,
-                               TmpKind + " " + RC->getName() + " subclass",
+                               TmpKind + ") and " + RC->getName() +
+                                   " is a subclass",
                                VisitFn, VisitedRCs);
 
-    // Visit each class that contains only subregisters of RC with a common
-    // subregister-index.
-    //
-    // More precisely, PossibleSubclass is a subreg-class iff Reg:SubIdx is in
-    // PossibleSubclass for all registers Reg from RC using any
-    // subregister-index SubReg
-    for (const auto &SubIdx : RegisterClassHierarchy.getSubRegIndices()) {
-      BitVector BV(RegisterClassHierarchy.getRegClasses().size());
-      PossibleSubclass.getSuperRegClasses(&SubIdx, BV);
-      if (BV.test(RC->EnumValue)) {
-        std::string TmpKind2 = (Twine(TmpKind) + " " + RC->getName() +
-                                " class-with-subregs: " + RC->getName())
-                                   .str();
-        VisitFn(&PossibleSubclass, TmpKind2);
-      }
+    // Visit each class that contains only subregisters of RC. The
+    // subregister-index can vary between them such.
+    SmallPtrSet<const CodeGenRegister *, 32> SubRegs = {
+        PossibleSubclass.getMembers().begin(),
+        PossibleSubclass.getMembers().end()};
+    for (const auto &SuperReg : RC->getMembers()) {
+      SubRegs.erase(SuperReg);
+      for (const auto &SubRegAndIdxOfSuperReg : SuperReg->getSubRegs())
+        SubRegs.erase(SubRegAndIdxOfSuperReg.second);
+    }
+
+    if (SubRegs.empty()) {
+      std::string TmpKind2 = (Twine(TmpKind) + " " +
+                              " every register is covered by " + RC->getName())
+                                 .str();
+      VisitFn(&PossibleSubclass, TmpKind2);
+    } else {
+      DEBUG(dbgs() << "Rejected " << PossibleSubclass.getName()
+                   << ", these registers are not covered by " << RC->getName()
+                   << ":\n");
+      for (const auto &Reg : SubRegs)
+        DEBUG(dbgs() << "  " << Reg->getName() << "\n");
     }
   }
 }
@@ -286,11 +299,13 @@
     for (const CodeGenRegisterClass *RC :
          Bank.getExplictlySpecifiedRegisterClasses(RegisterClassHierarchy)) {
       visitRegisterBankClasses(
-          RegisterClassHierarchy, RC, "explicit",
+          RegisterClassHierarchy, RC,
+          "because " + RC->getName() + " was explicitly-added",
           [&Bank](const CodeGenRegisterClass *RC, StringRef Kind) {
-            DEBUG(dbgs() << "Added " << RC->getName() << "(" << Kind << ")\n");
+            DEBUG(dbgs() << "Added " << RC->getName() << " (" << Kind << ")\n");
             Bank.addRegisterClass(RC);
-          }, VisitedRCs);
+          },
+          VisitedRCs);
     }
 
     Banks.push_back(Bank);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33352.99541.patch
Type: text/x-patch
Size: 3654 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170519/91d0e949/attachment.bin>