[PATCH] D78193: [Sparc] Fixes for the internal assembler

[Daniel Cederman via Phabricator via llvm-commits]

dcederman accepted this revision.
dcederman added a comment.
This revision is now accepted and ready to land.

It was some time since I worked with LLVM, but this looks good to me. It tries to fix a couple of things, so it would probably been better to divide it up into more than one patch and add some more tests, but this is fine. I compared the result with GAS for V8 and it matches.



================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:289
+  }
+  bool isShiftAmtImm6() const {
+    if (!isImm())
----------------
Is this for grouping? I think it looks better if there is an empty line between them.


================
Comment at: llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:384
+  }
+  void addShiftAmtImm6Operands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
----------------
Same here.


================
Comment at: llvm/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp:161
+
+  if (MO.isExpr()) {
+    const MCExpr *Expr = MO.getExpr();
----------------
Can this be false? In similar code there is usually an assert(MO.isExpr()) instead.


================
Comment at: llvm/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp:181
+
+  return MO.getImm();
+}
----------------
If we can reach this line then the operand is not an immediate so getImm() would not work.


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

https://reviews.llvm.org/D78193