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

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu marked an inline comment as done.
myhsu added inline comments.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:266
+  if (HwMode == -1)
+    OS << "  static const APInt InstBits[] = {\n";
+  else
----------------
jrtc27 wrote:
> myhsu wrote:
> > craig.topper wrote:
> > > I think this violates the no static global constructor rule?
> > no, `InstBits` (and `InstBits_<HW mode>`) is a local variable. Here is how it looks in the generated .inc file:
> > ```
> > void M68kMCCodeEmitter::getBinaryCodeForInstr(const MCInst &MI,
> >     SmallVectorImpl<MCFixup> &Fixups,
> >     APInt &Inst,
> >     APInt &Scratch,
> >     const MCSubtargetInfo &STI) const {
> >   static const APInt InstBits[] = {
> >     APInt::getZeroWidth(),
> >     APInt::getZeroWidth(),
> >     APInt::getZeroWidth(),
> > ...
> > ```
> A local variable with static storage duration, i.e. a static global that's scoped to the function
you're right. Then I think a potential solution will be using two integer arrays:
```
  static const unsigned Index[][2] = {
    {/*TotalBits*/36, /*Storage index*/0},  // FOO16
    {/*TotalBits*/80, /*Storage index*/1},  // FOO32
    ...
  };
  static const uint64_t Storage[] = {
    UINT64_C(53312), UINT64_C(53352), ...
  };
```
And use `Index` to indirectly access fixed values of each instruction in `Storage`.


================
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(
----------------
jrtc27 wrote:
> myhsu wrote:
> > craig.topper wrote:
> > > 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.
> > I don't think it will create a dangling reference in this particular case because DagInit::get actually [[ https://github.com/llvm/llvm-project/blob/93fbaa46c82abd2d54bce9a7c3b39b01c30220d6/llvm/lib/TableGen/Record.cpp#L2120 | copies ]] NewArgs' elements. But I'm changing to plain C array anyway.
> That's not Craig's point; `ArrayRef<Init *> NewArgs = {OperandName, LoBit, HiBit};` itself is broken, as NewArgs points to storage after that statement whose lifetime has ended (well, it's fine so long as you never use NewArgs, but then it's pointless), so by the time you've reached DagInit::get it's already too late, the storage is gone. Using an initialiser list for ArrayRef is solely for use within the same statement (i.e. things like `foo(ArrayRef({...}))`.
I see, that makes sense. Thanks for the explanation.


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

https://reviews.llvm.org/D115128