[RISCV] Handle 'c.addi x0, $imm' alias for c.nop using PseudoC_ADDI_NOP.

[topperc]

Add a missing tied constraint to PseudoC_ADDI_NOP.

It seemed better to handle all the c.addi aliases for c.nop in one place.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

Add a missing tied constraint to PseudoC_ADDI_NOP.

It seemed better to handle all the c.addi aliases for c.nop in one place.

---

Full diff: https://github.com/llvm/llvm-project/pull/150719.diff

3 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+7-2)
• (modified) llvm/lib/Target/RISCV/RISCVInstrInfoC.td (+5-8)
• (modified) llvm/test/MC/RISCV/rvc-hints-invalid.s (+1-1)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index a143d85f61ec2..84ef00781d800 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -3849,9 +3849,14 @@ bool RISCVAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
   switch (Inst.getOpcode()) {
   default:
     break;
-  case RISCV::PseudoC_ADDI_NOP:
-    emitToStreamer(Out, MCInstBuilder(RISCV::C_NOP));
+  case RISCV::PseudoC_ADDI_NOP: {
+    if (Inst.getOperand(2).getImm() == 0)
+      emitToStreamer(Out, MCInstBuilder(RISCV::C_NOP));
+    else
+      emitToStreamer(Out, MCInstBuilder(RISCV::C_NOP_HINT)
+                              .addOperand(Inst.getOperand(2)));
     return false;
+  }
   case RISCV::PseudoLLAImm:
   case RISCV::PseudoLAImm:
   case RISCV::PseudoLI: {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
index 8252a9b170eb3..8b5e7aa90e6cd 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
@@ -408,11 +408,13 @@ def C_ADDI : RVInst16CI<0b000, 0b01, (outs GPRNoX0:$rd_wb),
   let Constraints = "$rd = $rd_wb";
 }
 
-// Alternate syntax for c.nop. Converted to C_NOP by the assembler.
+// Alternate syntax for c.nop. Converted to C_NOP/C_NOP_HINT by the assembler.
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, isCodeGenOnly = 0,
     isAsmParserOnly = 1 in
-def PseudoC_ADDI_NOP : Pseudo<(outs GPRX0:$rd), (ins GPRX0:$rs1, immzero:$imm),
-                              [], "c.addi", "$rd, $imm">;
+def PseudoC_ADDI_NOP : Pseudo<(outs GPRX0:$rd), (ins GPRX0:$rs1, simm6:$imm),
+                              [], "c.addi", "$rd, $imm"> {
+  let Constraints = "$rs1 = $rd";
+}
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, isCall = 1,
     DecoderNamespace = "RV32Only", Defs = [X1],
@@ -698,11 +700,6 @@ def C_SRAI64_HINT : RVInst16CB<0b100, 0b01, (outs GPRC:$rd),
 // Assembler Pseudo Instructions
 //===----------------------------------------------------------------------===//
 
-let Predicates = [HasStdExtZca] in {
-// Just a different syntax for the c.nop hint: c.addi x0, simm6 vs c.nop simm6.
-def : InstAlias<"c.addi x0, $imm", (C_NOP_HINT simm6nonzero:$imm), 0>;
-}
-
 let Predicates = [HasStdExtC, HasStdExtZihintntl] in {
 def : InstAlias<"c.ntl.p1", (C_ADD_HINT X0, X2)>;
 def : InstAlias<"c.ntl.pall", (C_ADD_HINT X0, X3)>;
diff --git a/llvm/test/MC/RISCV/rvc-hints-invalid.s b/llvm/test/MC/RISCV/rvc-hints-invalid.s
index 2a7a6addd31ab..9af2915af56e5 100644
--- a/llvm/test/MC/RISCV/rvc-hints-invalid.s
+++ b/llvm/test/MC/RISCV/rvc-hints-invalid.s
@@ -5,7 +5,7 @@
 
 c.nop 0 # CHECK: :[[@LINE]]:7: error: immediate must be non-zero in the range [-32, 31]
 
-c.addi x0, 33 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
+c.addi x0, 33 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [-32, 31]
 
 c.li x0, 42 # CHECK: :[[@LINE]]:10: error: immediate must be an integer in the range [-32, 31]
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[lenary]

Why can't we do the following, which seems cleaner to me:

• Merge C_NOP and C_NOP_HINT (to C_NOP that takes a simm6 immediate, and prints as c.nop $imm)
• Add an alias c.nop to (C_NOP 0) which does print
• Add aliases for c.addi x0, $imm to (C_NOP $imm) which parse but don't print.

I think this would allow us to get rid of PseudoC_ADDI_NOP and the custom processInstruction code.

I'm overlooking something, maybe?

There's maybe one or two other places in the LLVM codebase which would have to be updated as they use C_NOP right now (without any operands), but I don't think that's a problem.

I don't think the new C_NOP would need to have tied operands, but maybe that's wrong?

[topperc]

• Add an alias c.nop to (C_NOP 0) which does print

-Mno-aliases would disable the printing and emit c.nop 0 which I don't think we want. It would be confusing to users and isn't accepted by binutils.

[lenary]

I see.

LGTM.

[lenary]

(I sort of wish the spec didn't say that c.nop was a new instruction, just that it was an alias, but there are always going to be edges like this).

[topperc]

(I sort of wish the spec didn't say that c.nop was a new instruction, just that it was an alias, but there are always going to be edges like this).

Yeah. binutils does treat it as an alias and I think prints c.addi x0, 0 with -Mno-aliases. I think that's a little less confusing to users than c.nop 0.