[PATCH] D64833: [Xtensa 7/10] Add Xtensa instruction printer.

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr added inline comments.


================
Comment at: llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp:33-34
+
+  if (!(SRE = dyn_cast<MCSymbolRefExpr>(Expr)))
+    assert(false && "Unexpected MCExpr type.");
+
----------------
arsenm wrote:
> Just use cast<>
Corrected


================
Comment at: llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp:89
+    int64_t Value = MI->getOperand(OpNum).getImm();
+    assert((Value >= -128 && Value <= 127) &&
+           "Invalid argument, value must be in ranges [-128,127]");
----------------
MaskRay wrote:
> `isInt<8>` from MathExtras.h
Corrected


================
Comment at: llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp:93
+  } else
+    printOperand(MI, OpNum, O);
+}
----------------
MaskRay wrote:
> Add braces. `then` uses braces and `else` should use, too.
Corrected


================
Comment at: llvm/lib/Target/Xtensa/MCTargetDesc/XtensaInstPrinter.cpp:100
+    int64_t Value = MI->getOperand(OpNum).getImm();
+    assert((Value >= -32768 && Value <= 32512 && ((Value & 0xFF) == 0)) &&
+           "Invalid argument, value must be multiples of 256 in range "
----------------
MaskRay wrote:
> isInt<16> from MathExtras.h
Corrected


================
Comment at: llvm/test/MC/Xtensa/elf-header.s:5
+# Xtensa: Format: elf32-xtensa
+# Xtensa: Arch: xtensa
+# Xtensa: AddressSize: 32bit
----------------
MaskRay wrote:
> Add `-NEXT` when applicable.
Corrected


================
Comment at: llvm/test/MC/Xtensa/xtensa-invalid.s:13
+# CHECK:      error: expected immediate in range [-128, 127]
+
+# imm8_sh8
----------------
saugustine wrote:
> andreisfr wrote:
> > saugustine wrote:
> > > It would be good to check instruction arguments out of order, and invalid register names.
> > You mean for each instruction implement extended tests with argument out of order and invalid register names?
> Probably just once for each format, but yes.
I added more tests for different instruction formats.


================
Comment at: llvm/test/MC/Xtensa/xtensa-valid.s:1
+# RUN: llvm-mc %s -triple=xtensa -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
----------------
MaskRay wrote:
> Split this into multiple files.
> 
> 
> MC/AArch64/armv8* and MC/LoongArch/Basic can give some insights how tests are organized.
Thank you for advice, I've splitted file to several files by commands groups.


================
Comment at: llvm/test/MC/Xtensa/xtensa-valid.s:4
+
+
+.align	4
----------------
saugustine wrote:
> It would be good here to comments marking which instruction formats  (RRR, RRI4, RRI8 and so on). re covered, as the difference between them is a common source of bugs. Perhaps next to one of the instructions.
> 
> I assume that eventually there will be comprehensive coverage of all the different formats.
> 
> 
> 
Added format information to each instruction.


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

https://reviews.llvm.org/D64833