[PATCH] D87634: [AVR] Improve AVR disassembly

[Dylan McKay via Phabricator via llvm-commits]

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

The code is looking nice, thanks for the patch @amikhalev!

I've checked over everything except the bitmasks/bit positions and left a few comments. I intend to validate the bitmasks sometime in the next few days.



================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:170
   let EncoderMethod = "encodeMemri";
+  let DecoderMethod = "decodeMemri";
 
----------------
Add a LLVM unit test that covers this (suggest somewhere in `test/MC/AVR`), perhaps using something like `inst-sts.s` as an example.


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:186
     let EncoderMethod = "encodeRelCondBrTarget<AVR::fixup_7_pcrel>";
+    let DecoderMethod = "decodeRelCondBrTarget<7>";
 }
----------------
Add a unit test for this new instruction decoder


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:194
     let EncoderMethod = "encodeRelCondBrTarget<AVR::fixup_13_pcrel>";
+    let DecoderMethod = "decodeRelCondBrTarget<13>";
 }
----------------
Add a unit test for this new instruction decoder


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:254
     let EncoderMethod = "encodeLDSTPtrReg";
+    let DecoderMethod = "decodeLDSTPtrReg";
+
----------------
Add a unit test for this new instruction decoder


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:258
+    // inconsistent bit.
+    let hasCompleteDecoder = 0;
 
----------------
I hadn't seen this TableGen flag before, nice


================
Comment at: llvm/lib/Target/AVR/AVRInstrInfo.td:270
     let EncoderMethod = "encodeLDSTPtrReg";
+    let DecoderMethod = "decodeLDSTPtrReg";
 
----------------
Add a unit test for this new instruction decoder


================
Comment at: llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp:95
+  switch (RegNo) {
+  case 0b00:
+    Register = AVR::R31R30; // Z
----------------
I too love using binary literal notation, but sadly I believe it is not fully supported by the minimum C++ toolchain versions LLVM supports. Specifically, binary literals are not mentioned in the C++ feature "whitelist"[1], nor can I spot any existing uses in LLVM via a quick grep, plus I remember committing a change that included binary literals not too long ago that failed on the buildbots due to lack of support.

Replace these binary literals with hex or decimal equivalents. If you want the binary representation to be shown, then it will have to be relegated to a code comment



  

  - [0] LLVM C++ feature whitelist https://releases.llvm.org/8.0.0/docs/CodingStandards.html#supported-c-11-language-and-library-features





================
Comment at: llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp:158
+  unsigned Reg;
+  if (YOrZ == 0) {
+    Reg = AVR::R31R30; // Z
----------------
Nitpick: `YOrZ` variable implies a boolean value. It is fairly clear in the definition and usage that it is always treated like a boolean value, but I recommend moving the `== 0` check up to the definition of `YOrZ` to make it clear right at the site of its initial declaration/definition. For example ` bool YOrZ = fieldFromInstruction(Insn, 6, 1) == 0`


================
Comment at: llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp:163
+  }
+  unsigned displacement = fieldFromInstruction(Insn, 0, 6);
+  Inst.addOperand(MCOperand::createReg(Reg));
----------------
Nitpick: Captialize `Displacement`


================
Comment at: llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp:209
+  // Sign extend
+  const uint64_t ValueMask = (1 << (Bits - 1)) - 1;
+  const uint64_t SignMask = static_cast<uint64_t>(-1) - ValueMask;
----------------
I haven't looked deep into this sign extension implementation yet but recommend replacing the hand rolled SExt with a call to one of the generalized methods in `MathExtras.h`, for example, `SignExtend64`


================
Comment at: llvm/lib/Target/AVR/Disassembler/AVRDisassembler.cpp:235
+
+  bool IsPredec = Opcode == AVR::LDRdPtrPd || Opcode == AVR::STPtrPdRr;
+  bool IsPostinc = Opcode == AVR::LDRdPtrPi || Opcode == AVR::STPtrPiRr;
----------------
Add some kind of assertion here to validate that you're covering all possible cases of input opcode without incorrectly falling back to `IsPredec==false && isPostinc==false`. The simplest solution might be done with `switch` but perhaps there is a more terse way to do it as this current code reads very nicely


================
Comment at: llvm/lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.cpp:13
 
+#include "AVRMCTargetDesc.h"
 #include "AVRELFStreamer.h"
----------------
Nitpick: Either place a new line after this "primary" include to give it special vertical positioning, or order it alphabetically along with the other includes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87634