[PATCH] D72574: [PowerPC][Future] Add pld and pstd to future CPU

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

I'm not convinced you really need the FixedLenDecoderEmitter.cpp changes. PPCInstrFormats.td has `bits<64> Inst` in the I2 class, but PPCDisassembler.cpp has InsnType as being uint32_t . InsnType needs to be able to hold your biggest instruction for the generated code to work correctly. I believe that if you changed:

> uint32_t Inst = ...;

in PPCDisassembler::getInstruction(), to:

> uint64_t Inst = ...;

then you will no longer need to change FixedLenDecoderEmitter.cpp as you'll be able to use shifts of up to 63.



================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1115
 
+  if (OpInfo.numFields() != 1 || OpInfo.begin()->Offset != 0) {
+    unsigned MaxOffset = 0;
----------------
The `OpInfo.begin()->Offset != 0` part doesn't look right to me at the moment. I think you're trying to guard against shift emitted by the code guarded by the `else if (OpInfo.numFields() != 1 || EF.Offset != 0)` on line 1133 (1120 in the old code) but here you only look at the first field whereas the original guard is checking all of them. Is there some sort order that guarantees that only the first field matters?


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1120
+    o.indent(Indentation);
+    o << "if ((sizeof(InsnType)*8) <= " << MaxOffset <<
+      ") { return MCDisassembler::Fail; }\n";
----------------
You can't really rely on sizeof(InsnType) being meaningful as InsnType is allowed to be an object. For example, that object might be using an APInt internally. You'll need to provide a template that can handle both integer and object InsnTypes.


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

https://reviews.llvm.org/D72574