[GlobalISel][TableGen] Add BitsPerByte value

[ehjogab]

Summary

In some targets, the smallest addressable unit does not consist of 8
bits. For such targets, the matcher code for G_SEXTLOAD and G_ZEXTLOAD
is incorrect and never yield a match. This patch addresses this problem
by introducing a new value BitsPerByte, thereby allowing the 8-bit
assumption to be removed from TableGen.

Change-Id: Ic6a3e6976a960c8ef65b4a7dbb065b2529d07dd9

[Paul-C-Anagnostopoulos]

llvm/utils/TableGen/GlobalISelEmitter.cpp

3814–3815

Could you fix the English in the comment above?

I should know, but which getSizeInBits() is this calling? There are two definitions, both with the 8 hardwired. One is in CodeGen/MachineMemOperand.h and the other in ir/DataLayout.h.

Ah, is it calling the one in LowLevelTypeImpl.h? That one doesn't have a hardwired 8, although the function getSizeInBytes does.

[ehjogab]

Fix lint warnings
Add testcase

[ehjogab]

I believe it is calling the one in LowLevelTypeImpl.h.

I agree that things become confusing when using hardcoded 8s in some places and then not in other places. Preferably, all hardcoded 8s should be replaced by something akin to this; we have done this in the codebase for our target but been unable to upstream the patch (for reasons unknown to me).

[lebedev.ri]

This should be an RFC to llvm-dev, because this tries to silently lift the restriction that byte is 8 bits,
which is prevalent through llvm. And i do mean silently, because this has been brought up before a number of times.

llvm/include/llvm/Target/Target.td

class Target {
   // AssemblyWriters - The AsmWriter instances available for this target.
   list<AsmWriter> AssemblyWriters = [DefaultAsmWriter];
 
   // AllowRegisterRenaming - Controls whether this target allows
   // post-register-allocation renaming of registers.  This is done by
   // setting hasExtraDefRegAllocReq and hasExtraSrcRegAllocReq to 1
   // for all opcodes if this flag is set to 0.
   int AllowRegisterRenaming = 0;
+
+  // BitsPerByte - Specifies the size (in bits) of the smallest addressable
+  // unit in the target. Typically this is 8 bits, which is the default value.
+  int BitsPerByte = 8;
 }
 
 //===----------------------------------------------------------------------===//
 // SubtargetFeature - A characteristic of the chip set.
 //
 class SubtargetFeature<string n, string a,  string v, string d,
                        list<SubtargetFeature> i = []> {
   // Name - Feature name.  Used by command line (-mattr=) to determine the

llvm/test/TableGen/GlobalISelEmitter-bits-per-byte.td

+// RUN: llvm-tblgen -gen-global-isel -optimize-match-table=false -I %p/../../include -I %p/Common %s -o - < %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def MyTargetISA : InstrInfo;
+def MyTarget : Target {
+  let InstructionSet = MyTargetISA;
+  let BitsPerByte = 16;
+}
+
+class MyTargetGenericInstruction : GenericInstruction {
+  let Namespace = "MyTarget";
+}
+
+def R0 : Register<"r0"> { let Namespace = "MyTarget"; }
+def GPR32 : RegisterClass<"MyTarget", [i32], 32, (add R0)>;
+def GPR32Op : RegisterOperand<GPR32>;
+def S0 : Register<"s0"> { let Namespace = "MyTarget"; }
+def GPR16 : RegisterClass<"MyTarget", [i16], 16, (add S0)>;
+def GPR16Op : RegisterOperand<GPR16>;
+
+class I<dag OOps, dag IOps, list<dag> Pat>
+  : Instruction {
+  let Namespace = "MyTarget";
+  let OutOperandList = OOps;
+  let InOperandList = IOps;
+  let Pattern = Pat;
+}
+
+def SEXTLOAD16 : I<(outs GPR32:$dst),
+                 (ins GPR16:$src), []>;
+
+// CHECK:      GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_SEXTLOAD,
+// CHECK-NEXT: GIM_CheckMemorySizeEqualTo, /*MI*/0, /*MMO*/0, /*Size*/1,
+// CHECK-NEXT: GIM_CheckAtomicOrdering, /*MI*/0, /*Order*/(int64_t)AtomicOrdering::NotAtomic,
+// CHECK-NEXT: // MIs[0] dst
+// CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT: // MIs[0] src
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/0, /*Op*/1, /*SizeInBits*/16,
+// CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR16RegClassID,
+// CHECK-NEXT: // (ld:{ *:[i32] } GPR16:{ *:[i16] }:$src)<<P:Predicate_unindexedload>><<P:Predicate_sextload>><<P:Predicate_sextloadi16>>  =>  (SEXTLOAD16:{ *:[i32] } GPR16:{ *:[i16] }:$src)
+// CHECK-NEXT: GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::SEXTLOAD16,
+// CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT: // GIR_Coverage, 0,
+// CHECK-NEXT: GIR_Done,
+def : Pat<(i32 (sextloadi16 GPR16:$src)),
+          (SEXTLOAD16 GPR16:$src)>;

llvm/utils/TableGen/CodeGenTarget.h

public:
   ///
   Record *getInstructionSet() const;
 
   /// getAllowRegisterRenaming - Return the AllowRegisterRenaming flag value for
   /// this target.
   ///
   bool getAllowRegisterRenaming() const;
 
+  /// getBitsPerByte - Return the BitsPerByte int value for this target.
+  ///
+  int getBitsPerByte() const;
+
   /// getAsmParser - Return the AssemblyParser definition for this target.
   ///
   Record *getAsmParser() const;
 
   /// getAsmParserVariant - Return the AssemblyParserVariant definition for
   /// this target.
   ///
   Record *getAsmParserVariant(unsigned i) const;

llvm/utils/TableGen/CodeGenTarget.cpp

 Record *CodeGenTarget::getInstructionSet() const {
   return TargetRec->getValueAsDef("InstructionSet");
 }
 
 bool CodeGenTarget::getAllowRegisterRenaming() const {
   return TargetRec->getValueAsInt("AllowRegisterRenaming");
 }
 
+int CodeGenTarget::getBitsPerByte() const {
+  return TargetRec->getValueAsInt("BitsPerByte");
+}
+
 /// getAsmParser - Return the AssemblyParser definition for this target.
 ///
 Record *CodeGenTarget::getAsmParser() const {
   std::vector<Record*> LI = TargetRec->getValueAsListOfDefs("AssemblyParsers");
   if (AsmParserNum >= LI.size())
     PrintFatalError("Target does not have an AsmParser #" +
                     Twine(AsmParserNum) + "!");
   return LI[AsmParserNum];

llvm/utils/TableGen/GlobalISelEmitter.cpp

if (Predicate.isLoad() || Predicate.isStore() || Predicate.isAtomic()) {
     if (Predicate.getMemoryVT() != nullptr) {
       Optional<LLTCodeGen> MemTyOrNone =
           MVTToLLT(getValueType(Predicate.getMemoryVT()));
 
       if (!MemTyOrNone)
         return failedImport("MemVT could not be converted to LLT");
 
       // MMO's work in bytes so we must take care of unusual types like i1
       // don't round down.
-      unsigned MemSizeInBits =
-          llvm::alignTo(MemTyOrNone->get().getSizeInBits(), 8);
-
-      InsnMatcher.addPredicate<MemorySizePredicateMatcher>(0,
-                                                           MemSizeInBits / 8);
+      unsigned MemSizeInBits = llvm::alignTo(MemTyOrNone->get().getSizeInBits(),
+                                             Target.getBitsPerByte());
+      unsigned RoundedMemSize = MemSizeInBits / Target.getBitsPerByte();
+      InsnMatcher.addPredicate<MemorySizePredicateMatcher>(0, RoundedMemSize);
       return InsnMatcher;
     }
   }
 
   if (Predicate.isLoad() || Predicate.isStore()) {
     // No check required. A G_LOAD/G_STORE is an unindexed load.
     if (Predicate.isUnindexed())
       return InsnMatcher;