[TableGen] Fix negation of simple predicates

[evandro]

Summary

Simple predicates, such as those defined by CheckRegOperandSimple or CheckImmOperandSimple, were not being negated when used with CheckNot.

This change fixes this issue by defining the previously declared methods to handle simple predicates.

[andreadb]

Hi Evandro.

The two new methods added by your patch are mostly equivalent to expandCheckImmOperand and expandCheckRegOperand.
Rather than introducing those new methods, I think a better fix is to add checks for null/empty operands in expandCheckImmOperand and expandCheckRegOperand.

Basically, I am thinking about something like this:

Index: TableGen/PredicateExpander.cpp
===================================================================
--- TableGen/PredicateExpander.cpp      (revision 347858)
+++ TableGen/PredicateExpander.cpp      (working copy)
@@ -31,14 +31,16 @@
 }

 void PredicateExpander::expandCheckImmOperand(raw_ostream &OS, int OpIndex,
                                               StringRef ImmVal,
                                               StringRef FunctionMapper) {
   if (!FunctionMapper.empty())
     OS << FunctionMapper << "(";
+  if (ImmVal.empty() && shouldNegate())
+    OS << '!';
   OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
      << ").getImm()";

   OS << (FunctionMapper.empty() ? "" : ")");
   if (ImmVal.empty())
     return;
   OS << (shouldNegate() ? " != " : " == ") << ImmVal;
@@ -47,14 +49,16 @@
 void PredicateExpander::expandCheckRegOperand(raw_ostream &OS, int OpIndex,
                                               const Record *Reg,
                                               StringRef FunctionMapper) {
   assert(Reg->isSubClassOf("Register") && "Expected a register Record!");

   if (!FunctionMapper.empty())
     OS << FunctionMapper << "(";
+  if (!Reg && shouldNegate())
+    OS << '!';
   OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
      << ").getReg()";
   OS << (FunctionMapper.empty() ? "" : ")");
   if (!Reg)
     return;
   OS << (shouldNegate() ? " != " : " == ");
   const StringRef Str = Reg->getValueAsString("Namespace");

As a side note: your patch made me realize that the declarations of expandCheckRegOperandSimple and expandCheckImmOperandSimple in the header file are pretty much dead. In case, (if you agree with this alternative approach), your patch should be able to remove them from the header file.

-Andrea

[evandro]

I'm all for reusing code, but this is not any sophisticated algorithm, but mere streaming of data. I rather keep the additional methods and simplify the exiting ones to eliminate their special cases for the sake of maintenance. After all, complicating them is what resulted in this issue.

[andreadb]

In D55089#1314992, @evandro wrote:

I'm all for reusing code, but this is not any sophisticated algorithm, but mere streaming of data. I rather keep the additional methods and simplify the exiting ones to eliminate their special cases for the sake of maintenance. After all, complicating them is what resulted in this issue.

Fair enough.

LGTM

llvm/utils/TableGen/PredicateExpander.cpp

OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
      << ").getImm()";
 
   OS << (FunctionMapper.empty() ? "" : ")");
   if (ImmVal.empty())
     return;
   OS << (shouldNegate() ? " != " : " == ") << ImmVal;
 }
 
+void PredicateExpander::expandCheckImmOperandSimple(raw_ostream &OS,
+                                                    int OpIndex,
+                                                    StringRef FunctionMapper) {
+  if (shouldNegate())
+    OS << "!";
+  if (!FunctionMapper.empty())
+    OS << FunctionMapper << "(";
+  OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
+     << ").getImm()";
+  OS << (FunctionMapper.empty() ? " " : ") ");
+}
+
 void PredicateExpander::expandCheckRegOperand(raw_ostream &OS, int OpIndex,
                                               const Record *Reg,
                                               StringRef FunctionMapper) {
   assert(Reg->isSubClassOf("Register") && "Expected a register Record!");
 
   if (!FunctionMapper.empty())
     OS << FunctionMapper << "(";
   OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
      << ").getReg()";
   OS << (FunctionMapper.empty() ? "" : ")");
   if (!Reg)
     return;
   OS << (shouldNegate() ? " != " : " == ");
   const StringRef Str = Reg->getValueAsString("Namespace");
   if (!Str.empty())
     OS << Str << "::";
   OS << Reg->getName();
 }
 
+
+void PredicateExpander::expandCheckRegOperandSimple(raw_ostream &OS,
+                                                    int OpIndex,
+                                                    StringRef FunctionMapper) {
+  if (shouldNegate())
+    OS << "!";
+  if (!FunctionMapper.empty())
+    OS << FunctionMapper << "(";
+  OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
+     << ").getReg()";
+  if (!FunctionMapper.empty())
+    OS << ")";
+}
+
 void PredicateExpander::expandCheckInvalidRegOperand(raw_ostream &OS,
                                                      int OpIndex) {
   OS << "MI" << (isByRef() ? "." : "->") << "getOperand(" << OpIndex
      << ").getReg() " << (shouldNegate() ? "!= " : "== ") << "0";
 }
 
 void PredicateExpander::expandCheckSameRegOperand(raw_ostream &OS, int First,
                                                   int Second) {
if (Rec->isSubClassOf("CheckIsImmOperand"))
     return expandCheckIsImmOperand(OS, Rec->getValueAsInt("OpIndex"));
 
   if (Rec->isSubClassOf("CheckRegOperand"))
     return expandCheckRegOperand(OS, Rec->getValueAsInt("OpIndex"),
                                  Rec->getValueAsDef("Reg"),
                                  Rec->getValueAsString("FunctionMapper"));
 
   if (Rec->isSubClassOf("CheckRegOperandSimple"))
-    return expandCheckRegOperand(OS, Rec->getValueAsInt("OpIndex"),
-                                 nullptr,
+    return expandCheckRegOperandSimple(OS, Rec->getValueAsInt("OpIndex"),
                                        Rec->getValueAsString("FunctionMapper"));
 
   if (Rec->isSubClassOf("CheckInvalidRegOperand"))
     return expandCheckInvalidRegOperand(OS, Rec->getValueAsInt("OpIndex"));
 
   if (Rec->isSubClassOf("CheckImmOperand"))
     return expandCheckImmOperand(OS, Rec->getValueAsInt("OpIndex"),
                                  Rec->getValueAsInt("ImmVal"),
                                  Rec->getValueAsString("FunctionMapper"));
 
   if (Rec->isSubClassOf("CheckImmOperand_s"))
     return expandCheckImmOperand(OS, Rec->getValueAsInt("OpIndex"),
                                  Rec->getValueAsString("ImmVal"),
                                  Rec->getValueAsString("FunctionMapper"));
 
   if (Rec->isSubClassOf("CheckImmOperandSimple"))
-    return expandCheckImmOperand(OS, Rec->getValueAsInt("OpIndex"), "", 
+    return expandCheckImmOperandSimple(OS, Rec->getValueAsInt("OpIndex"),
                                        Rec->getValueAsString("FunctionMapper"));
 
   if (Rec->isSubClassOf("CheckSameRegOperand"))
     return expandCheckSameRegOperand(OS, Rec->getValueAsInt("FirstIndex"),
                                      Rec->getValueAsInt("SecondIndex"));
 
   if (Rec->isSubClassOf("CheckNumOperands"))
     return expandCheckNumOperands(OS, Rec->getValueAsInt("NumOps"));