[PATCH] D115128: [TableGen][CodeEmitter] Introducing the VarLenCodeEmitterGen infrastructure

[Craig Topper via Phabricator via llvm-commits]

craig.topper requested changes to this revision.
craig.topper added a comment.
This revision now requires changes to proceed.

Marking as changes required to flag the dangerous ArrayRef usage before this gets committed.



================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:404
+      }))
+    VarLenCodeEmitterGen(Records).run(o);
+  else {
----------------
Given that only `run` is called on the VarLenCodeEmitterGen object, it might make sense to move the VarLenCodeEmitterGen class defintion into the cpp file and only expose a free function wrapper.


================
Comment at: llvm/utils/TableGen/CodeEmitterGen.cpp:405
+    VarLenCodeEmitterGen(Records).run(o);
+  else {
+    const CodeGenHwModes &HWM = Target.getHwModes();
----------------
Curly brace the if body for consistency with the else


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:71
+    bool Reverse = Op == "descend";
+    int i = Reverse ? DI->getNumArgs() - 1 : 0,
+        e = Reverse ? -1 : DI->getNumArgs(), s = Reverse ? -1 : 1;
----------------
It might be better to not use the comma operator here and just have 3 variable declarations on 3 different lines.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:83
+        Segments.push_back({1, BI});
+      } else if (const auto *SubDI = dyn_cast<DagInit>(Arg))
+        buildRec(SubDI);
----------------
I believe by the third paragraph of this section https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements this block and the follow else should have curly braces to keep consistency.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:126
+      // Normalization: Hi bit should always be the second argument.
+      ArrayRef<Init *> NewArgs = {OperandName, LoBit, HiBit};
+      Segments.push_back(
----------------
This code is dangerous. It will create a dangling reference to an initializer_list that goes out of scope when the line ends. Use a plain C array here.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.h:16
+#define LLVM_UTILS_TABLEGEN_VARLENCODEEMITTERGEN_H
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
----------------
I think there is typically a blank line after the include guards.


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

https://reviews.llvm.org/D115128