[GlobalISel] ] Add a generic machine opcode for ADD.

[qcolombet]

Summary

Hi,

The selection process being split into separate passes (will happen soon, right? ;)), we need generic opcodes to translate the LLVM IR to target independent code.

This patch adds an opcode for addition: gADD.

The patch is pretty straight forward, but I still opted for pre-commit review as I want to be sure everyone agrees on the basic naming convention: g<OPCODE>, where g stands for generic.

From this point, the idea is to add the opcode as we need them and also, just add the ones that we think are necessary. Anyhow, we need at least one to start prototyping the IRTranslator.

Cheers,
-Quentin

[MatzeB]

My vote would go for just 'ADD'. The other generic instruction don't have a prefix either.

I assume the reason for a 'g' prefix are:

• Make it obvious that the opcode needs to be replaced after GISel: having a verifier check that is enough IMO
• Differentiating it from target opcodes: This may be a better reason, though so far the targets I looked at had 'r','i',32'... suffixes on their instructions.

[qcolombet]

Following a discussion with Matthias on IRC, here are a few points why I chose gADD instead of ADD:

• I want a visual distinction between generic opcode, like COPY, and generic opcode that are used only by GlobalISel. The latter must not occur after the select pass. (Will be enforced by the verifier.)
• I expect targets to have ADD, SUB, etc. operators and I don’t want to conflict with those names, I also want a clear visual distinction, e.g., gADD vs. ADDrr is better than ADD vs. ADDrr IMO.

Q.

[qcolombet]

Forgot to put the list in CC!

[qcolombet]

Ping.

[arsenm]

I would prefer G_ instead of the only lowercase letter for an instruction name

[qcolombet]

So far we have three proposals for GISel opcodes, all of them got one vote:

1. g<opcode>, e.g., gADD
2. <opcode>, e.g., ADD
3. G_<opcode>, e.g., G_ADD

Which one is your favorite?

I am going to go for #1 if I do not get other vote to break the tie :).

Cheers,
-Quentin

[reames]

Either (1) or (3) would be good. (2) is ambiguous.

Philip

[qcolombet]

Thanks all for the feedbacks.

Renamed accordingly.

[qcolombet]

Hi,

Ok to commit?

Thanks,
-Quentin

[arsenm]

LGTM

include/llvm/Target/Target.td

def LOCAL_ESCAPE : Instruction {
   let hasCtrlDep = 1;
 }
 def FAULTING_LOAD_OP : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);
   let usesCustomInserter = 1;
   let mayLoad = 1;
 }
+
+// Generic opcode used by the IRTranslator.
+// After ISel, this opcode should not appear.
+def gADD : Instruction {
+  let OutOperandList = (outs unknown:$dst);
+  let InOperandList = (ins unknown:$src1, unknown:$src2);
+  let AsmString = "";
+  let hasSideEffects = 0;
+  let isCommutable = 1;
+}
+// TODO: Add the other generic opcodes.
 }
 
 //===----------------------------------------------------------------------===//
 // AsmParser - This class can be implemented by targets that wish to implement
 // .s file parsing.
 //
 // Subtargets can have multiple different assembly parsers (e.g. AT&T vs Intel
 // syntax on X86 for example).

include/llvm/Target/TargetOpcodes.h

enum {
   LOCAL_ESCAPE = 21,
 
   /// Loading instruction that may page fault, bundled with associated
   /// information on how to handle such a page fault.  It is intended to support
   /// "zero cost" null checks in managed languages by allowing LLVM to fold
   /// comparisons into existing memory operations.
   FAULTING_LOAD_OP = 22,
 
+  /// The following generic opcodes are not supposed to appear after ISel.
+  /// This is something we might want to relax, but for now, this is convenient
+  /// to produce diagnostic.
+  PRE_ISEL_GENERIC_OPCODE_START,
+  // Generic opcodes used before ISel are here.
+
+  /// Generic ADD instruction. This is an integer add.
+  gADD = PRE_ISEL_GENERIC_OPCODE_START,
+  // TODO: Add more generic opcodes as we move along.
+  // FIXME: Right now, we have to manually add any new opcode in
+  // CodeGenTarget.cpp for TableGen to pick them up.
+  // Moreover the order must match.
+
+  /// Marker for the end of the generic opcode.
+  /// This is used to check if an opcode is in the range of the
+  /// generic opcodes.
+  PRE_ISEL_GENERIC_OPCODE_END,
+
   /// BUILTIN_OP_END - This must be the last enum value in this list.
   /// The target-specific post-isel opcode values start here.
-  GENERIC_OP_END = FAULTING_LOAD_OP,
+  GENERIC_OP_END = PRE_ISEL_GENERIC_OPCODE_END,
 };
 } // end namespace TargetOpcode
 } // end namespace llvm
 
 #endif

test/TableGen/trydecode-emission.td

def InstB : TestInstruction {
   let AsmString = "InstB";
   let DecoderMethod = "DecodeInstB";
   let hasCompleteDecoder = 0;
 }
 
 // CHECK:      /* 0 */       MCD::OPC_ExtractField, 4, 4,  // Inst{7-4} ...
 // CHECK-NEXT: /* 3 */       MCD::OPC_FilterValue, 0, 14, 0, // Skip to: 21
 // CHECK-NEXT: /* 7 */       MCD::OPC_CheckField, 2, 2, 0, 5, 0, // Skip to: 18
-// CHECK-NEXT: /* 13 */      MCD::OPC_TryDecode, 24, 0, 0, 0, // Opcode: InstB, skip to: 18
-// CHECK-NEXT: /* 18 */      MCD::OPC_Decode, 23, 1, // Opcode: InstA
+// CHECK-NEXT: /* 13 */      MCD::OPC_TryDecode, 25, 0, 0, 0, // Opcode: InstB, skip to: 18
+// CHECK-NEXT: /* 18 */      MCD::OPC_Decode, 24, 1, // Opcode: InstA
 // CHECK-NEXT: /* 21 */      MCD::OPC_Fail,
 
 // CHECK: if (DecodeInstB(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }

test/TableGen/trydecode-emission2.td

def InstB : TestInstruction {
   let hasCompleteDecoder = 0;
 }
 
 // CHECK:      /* 0 */       MCD::OPC_ExtractField, 2, 1,  // Inst{2} ...
 // CHECK-NEXT: /* 3 */       MCD::OPC_FilterValue, 0, 29, 0, // Skip to: 36
 // CHECK-NEXT: /* 7 */       MCD::OPC_ExtractField, 5, 3,  // Inst{7-5} ...
 // CHECK-NEXT: /* 10 */      MCD::OPC_FilterValue, 0, 22, 0, // Skip to: 36
 // CHECK-NEXT: /* 14 */      MCD::OPC_CheckField, 0, 2, 3, 5, 0, // Skip to: 25
-// CHECK-NEXT: /* 20 */      MCD::OPC_TryDecode, 24, 0, 0, 0, // Opcode: InstB, skip to: 25
+// CHECK-NEXT: /* 20 */      MCD::OPC_TryDecode, 25, 0, 0, 0, // Opcode: InstB, skip to: 25
 // CHECK-NEXT: /* 25 */      MCD::OPC_CheckField, 3, 2, 0, 5, 0, // Skip to: 36
-// CHECK-NEXT: /* 31 */      MCD::OPC_TryDecode, 23, 1, 0, 0, // Opcode: InstA, skip to: 36
+// CHECK-NEXT: /* 31 */      MCD::OPC_TryDecode, 24, 1, 0, 0, // Opcode: InstA, skip to: 36
 // CHECK-NEXT: /* 36 */      MCD::OPC_Fail,
 
 // CHECK: if (DecodeInstB(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }
 // CHECK: if (DecodeInstA(MI, insn, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }

test/TableGen/trydecode-emission3.td

def InstB : TestInstruction {
   let Inst{1-0} = op;
   let OutOperandList = (outs InstBOp:$op);
   let AsmString = "InstB";
 }
 
 // CHECK:      /* 0 */       MCD::OPC_ExtractField, 4, 4,  // Inst{7-4} ...
 // CHECK-NEXT: /* 3 */       MCD::OPC_FilterValue, 0, 14, 0, // Skip to: 21
 // CHECK-NEXT: /* 7 */       MCD::OPC_CheckField, 2, 2, 0, 5, 0, // Skip to: 18
-// CHECK-NEXT: /* 13 */      MCD::OPC_TryDecode, 24, 0, 0, 0, // Opcode: InstB, skip to: 18
-// CHECK-NEXT: /* 18 */      MCD::OPC_Decode, 23, 1, // Opcode: InstA
+// CHECK-NEXT: /* 13 */      MCD::OPC_TryDecode, 25, 0, 0, 0, // Opcode: InstB, skip to: 18
+// CHECK-NEXT: /* 18 */      MCD::OPC_Decode, 24, 1, // Opcode: InstA
 // CHECK-NEXT: /* 21 */      MCD::OPC_Fail,
 
 // CHECK: if (DecodeInstBOp(MI, tmp, Address, Decoder) == MCDisassembler::Fail) { DecodeComplete = false; return MCDisassembler::Fail; }

utils/TableGen/CodeGenTarget.cpp

if (!Rec || I == Insts.end())
     PrintFatalError(Twine("Could not find '") + Name + "' instruction!");
   return I->second.get();
 }
 
 /// \brief Return all of the instructions defined by the target, ordered by
 /// their enum value.
 void CodeGenTarget::ComputeInstrsByEnum() const {
   // The ordering here must match the ordering in TargetOpcodes.h.
+  // FIXME: It would be nice to have the opcode directly extracted
+  // to avoid potential errors. At the very, least a compile time
+  // error would be appreciated if the order does not match.
   static const char *const FixedInstrs[] = {
       "PHI",          "INLINEASM",     "CFI_INSTRUCTION",  "EH_LABEL",
       "GC_LABEL",     "KILL",          "EXTRACT_SUBREG",   "INSERT_SUBREG",
       "IMPLICIT_DEF", "SUBREG_TO_REG", "COPY_TO_REGCLASS", "DBG_VALUE",
       "REG_SEQUENCE", "COPY",          "BUNDLE",           "LIFETIME_START",
       "LIFETIME_END", "STACKMAP",      "PATCHPOINT",       "LOAD_STACK_GUARD",
       "STATEPOINT",   "LOCAL_ESCAPE",   "FAULTING_LOAD_OP",
+      // Generic opcodes for GlobalISel start here.
+      "gADD",
       nullptr};
   const auto &Insts = getInstructions();
   for (const char *const *p = FixedInstrs; *p; ++p) {
     const CodeGenInstruction *Instr = GetInstByName(*p, Insts, Records);
     assert(Instr && "Missing target independent instruction");
     assert(Instr->Namespace == "TargetOpcode" && "Bad namespace");
     InstrsByEnum.push_back(Instr);
   }