[PATCH] D113475: [llvm-tblgen][RISCV] Make llvm-tblgen RISCVCompressInstEmitter to be common infra across different targets

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/utils/TableGen/CompressInstEmitter.cpp:1
+//===------ CompressInstEmitter.cpp - Generator for Compression Inst-------===//
+//
----------------
Generator for Compression Inst doesn't make sense; Inst Compression does, or you could leave it as Generator for Compression given it used to be Generator for RISCV Compression


================
Comment at: llvm/utils/TableGen/CompressInstEmitter.cpp:35
+//
+// The result is an auto-generated header file which exports two functions
+// for compressing/uncompressing MCInst instructions, plus
----------------
This used to say what the name of the header is


================
Comment at: llvm/utils/TableGen/CompressInstEmitter.cpp:558
+
+  StringRef Namespace = Target.getName();
+
----------------
Namespace is not as informative, and does not make sense to prepend to "Subtarget". Call this TargetName instead (see the various methods in SubtargetFeatureInfo that take a TargetName and use it both as a prefix for Subtarget and as a namespace).


================
Comment at: llvm/utils/TableGen/CompressInstEmitter.cpp:419
+  Record *Operator = SourceDag->getOperatorAsDef(Rec->getLoc());
+  if (!Operator->isSubClassOf("Inst32"))
+    PrintFatalError(Rec->getLoc(), "Input instruction '" + Operator->getName() +
----------------
zixuan-wu wrote:
> craig.topper wrote:
> > Are these errors the only reasons we need the Inst32 and Inst16 classes to be introduced in Target.td? Can we check the size relationships instead? If we're making this generic it doesn't seem like we should be hardcoding 16 and 32.
> Hmm. Good advice. I think checking size is better way to tell the instruction width.
This still hard-codes 16 and 32 as compressed and uncompressed instruction sizes. When it was RISC-V-specific that was fine, but this is now a generic pass and, whilst the only consumers are architectures where those happen to be the two instruction widths involved, that won't necessarily be true of other architectures. As far as I can tell nothing in this generator cares what the sizes are; just change it to enforce that the compressed instruction is strictly smaller than the uncompressed instruction, i.e. it's actually compressing.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113475/new/

https://reviews.llvm.org/D113475