[PATCH] D88390: [M68k] (Patch 4/8) MC layer and object file support

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/CMakeLists.txt:7
+  M68kELFObjectWriter.cpp
+  M68kInstPrinter.cpp
 
----------------
Fix sorting


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp:81
+  // Mask is always 16 bits
+  assert((Mask & 0xFFFF) == Mask);
+
----------------
(style) Move comment into assert message
```
assert((Mask & 0xFFFF) == Mask && "Mask should be 16 bits");
```


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.h:44
+private:
+  void printOperand(const MCInst *MI, unsigned opNum, raw_ostream &O);
+  void printImmediate(const MCInst *MI, int opNum, raw_ostream &O);
----------------
why is this unsigned when all the other methods that have 'opNum' use int ?


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp:178
+  assert(Size + Offset <= 64 && "Value does not fit");
+  assert(isUIntN(Size, Val));
+
----------------
Merge asserts:
```
  assert((Size + Offset <= 64) && isUIntN(Size, Val) && "Value does not fit");
```


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp:335
+  const uint8_t *Beads = getGenInstrBeads(MI);
+  if (!*Beads) {
+    llvm_unreachable("*** Instruction does not have Beads defined");
----------------
Should we test for null pointer as well here?
```
if (!Beads || !*Beads) {
```


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp:4
-// LLVM<Target>CodeGen need to be present and some APIs
-// like `LLVMInitializeM68kTarget` need to be there
 
----------------
We should probably have the boilerplate comments in the initial version of the file?


================
Comment at: llvm/lib/Target/M68k/TargetInfo/M68kTargetInfo.cpp:4
-// LLVM<Target>CodeGen need to be present and some APIs
-// like `LLVMInitializeM68kTarget` need to be there
 
----------------
We should probably have the boilerplate comments in the initial version of the file?


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

https://reviews.llvm.org/D88390