[PATCH] D42834: [RISCV] Implement c.lui immedate operand constraint

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Shiva. I've left comments inline regarding the naming convention for this operand type, and the printing policy. As I say, I think we should let c.lui's operand print in the same way as lui for now. Consistently printing hex for lui and c.lui (as I think gnu objdump does) probably makes sense, but lets do that in a separate patch and do it for both instructions at the same time.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:276
 
-  bool isUImm6NonZero() const {
+  bool isUImm6CLUI() const {
     int64_t Imm;
----------------
Given that 'UImm6' is misleading I'd be tempted to just call this and the other related methods`isCLUIImm`. If preferring a `isUImm` naming scheme, `isUImm20CLUI` would actually be most consistent with the naming convention used for other asm operands, but that is also confusing. c.lui is a weird special case, so lets give it a name that reflects that.


================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:92
+
+void RISCVInstPrinter::printCLUIImm(const MCInst *MI, unsigned OpNo,
+                                    const MCSubtargetInfo &STI,
----------------
I don't think that the printing policy for c.lui should differ from lui. i.e. if we print it as '1048544' for lui then we should do the same for c.lui. We might later want to change the cases where hex is printed rather than decimal, but that would be better in a separate patch and applied consistently.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:135
+    unsigned Res = MO.getImm();
+    assert((isUInt<5>(Res) || ((Res >= 0xfffe0 && Res <= 0xfffff))) &&
+           "Not in c.lui immediate range!");
----------------
Drop unnecessary parens:

`((Res >= 0xfffe0 && Res <= 0xfffff))` -> `(Res >= 0xfffe0 && Res <= 0xfffff)`


Repository:
  rL LLVM

https://reviews.llvm.org/D42834