[PATCH] [RFC] Deprecated feature in TableGen

[Jim Grosbach]

  Diagnostic machinery is the only substantive bit, I think. The rest is various tidy-up sort of nitpickery.


================
Comment at: include/llvm/MC/MCInstrDesc.h:165
@@ -160,1 +164,3 @@
 
+  bool isDeprecated(MCInst &MI, MCSubtargetInfo &STI) const {
+    if (CustomDeprecator)
----------------
Add a \brief summary.


================
Comment at: include/llvm/MC/MCInstrDesc.h:167
@@ +166,3 @@
+    if (CustomDeprecator)
+      return CustomDeprecator(MI, STI);
+    return (STI.getFeatureBits() & Deprecated) != 0;
----------------
Possibly add a comment along the lines of "Some instructions are deprecated via more complex predicates than just subtarget features. Check for those first."

================
Comment at: include/llvm/MC/MCInstrDesc.h:151
@@ -148,1 +150,3 @@
+  typedef bool (*DepFN)(MCInst&, MCSubtargetInfo&);
+  DepFN CustomDeprecator;        // Holds an optional deprecator method
 
----------------
I'd prefer clearer names for these that are a bit more self-explanatory. Perhaps something like:
DeprecationFeatureMask and ComplexDeprecationPredicate?

No need for the typedef since it's only used here. Just declare the function pointer directly.

================
Comment at: include/llvm/Support/TargetRegistry.h:1146
@@ -1144,1 +1145,3 @@
+                                        const MCInstrInfo &MI) {
+      return new MCAsmParserImpl(STI, P, MI);
     }
----------------
s/MI/MII/

================
Comment at: include/llvm/Target/Target.td:455
@@ -453,1 +454,3 @@
+  /// A custom method to determine if an instruction is deprecated or not.
+  string CustomDeprecatedMethod = "";
 }
----------------
Using the same name here as above in the C++ will help make grepping the code to find things easier.

================
Comment at: include/llvm/Target/Target.td:1017
@@ +1016,3 @@
+class Deprecated<SubtargetFeature dep> {
+  SubtargetFeature Dep = dep;
+}
----------------
No need to abbreviate for the attribute itself (the arg name is fine shortened). Go ahead and use the full name and match it to the C++ name for searchability.

================
Comment at: lib/Target/ARM/ARMInstrInfo.td:4762
@@ -4761,2 +4761,3 @@
 
+let CustomDeprecatedMethod = "MCRIsDeprecated" in {
 def MCR : MovRCopro<"mcr", 0 /* from ARM core register to coprocessor */,
----------------
s/MCRIsDeprecated/isMCRDeprecated/

Do the Thumb2 encodings also need something similar, or is the deprecation just for the ARM encodings?

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5299
@@ -5298,3 +5283,1 @@
-}
-
 // FIXME: We would really like to be able to tablegen'erate this.
----------------
Yay for minuses. Nice cleanup.

================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:3035
@@ -3021,1 +3034,3 @@
+  }
+
   OS << "    return Match_Success;\n";
----------------
It'd be good to keep the ability to say why/when something is deprecated. For example,
"MCR encoding blah-blah-blah is deprecated on ARMv8 and newer."

For the subtarget feature driven deprecation, we can use, for now, the subtarget name like we do for the "instruction requires XYZ" diagnostics. For the custom predicate, perhaps instead of naming just the predicate method, do it more like the AsmMatcher names where we materialize multiple function signatures that the target's asmparser must implement. In this case, it would be the predicate and a diagnostic string generator. Something like,

let ComplexDeprecationPredicate="MCR" in {
...
}

TableGen would use functions named isMCRDeprecated() and getMCRDeprecationString().

================
Comment at: utils/TableGen/CodeGenInstruction.h:252
@@ +251,3 @@
+    std::string DeprecatedReason;
+    bool HasCustomDeprecator;
+
----------------
HasCustomDeprecationPredicate

================
Comment at: utils/TableGen/CodeGenInstruction.h:251
@@ -250,1 +250,3 @@
 
+    std::string DeprecatedReason;
+    bool HasCustomDeprecator;
----------------
Name doesn't sound quite right, but I don't have any better suggestions.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp:32
@@ -31,1 +31,3 @@
 
+static bool MCRIsDeprecated(llvm::MCInst &MI, llvm::MCSubtargetInfo &STI) {
+  // Checks for the deprecated CP15ISB encoding:
----------------
Not really inherent in your patch, but the explicit namespacing here is really odd. Perhaps the "using namespace llvm" should be moved up?

================
Comment at: utils/TableGen/CodeGenInstruction.cpp:356
@@ -340,1 +355,3 @@
+    DeprecatedReason = Dep->getValue()->getAsString();
+  }*/
 }
----------------
Looks like a remnant from previous work-in-progress. I assume the commented out bit can just be deleted entirely?

================
Comment at: utils/TableGen/CodeGenInstruction.cpp:351
@@ +350,3 @@
+      DeprecatedReason = "";
+  }
+/*
----------------
This could use some comments explaining what's happening. It'll be somewhat redundant with comments elsewhere, but that's OK.


http://llvm-reviews.chandlerc.com/D1399