[PATCH] D50046: [RISCV] Add InstAlias definitions for add, and, xor, or, sll, srl, sra, slt and sltu with immediate

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Kito, I've added a couple of comments. I think the main issue is the pattern for the shift amount immediate, which means invalid assembly is accepted.



================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:562-567
+def : InstAlias<"sll $rd, $rs1, $imm12",
+                (SLLI  GPR:$rd, GPR:$rs1, simm12:$imm12)>;
+def : InstAlias<"srl $rd, $rs1, $imm12",
+                (SRLI  GPR:$rd, GPR:$rs1, simm12:$imm12)>;
+def : InstAlias<"sra $rd, $rs1, $imm12",
+                (SRAI  GPR:$rd, GPR:$rs1, simm12:$imm12)>;
----------------
These should use uimmlog2xlen for the shift amount. When fixing this, could you please also add tests that demonstrate the range checks are correct (adding to rv32i-aliases-invalid.s and rv64i-aliases-invalid.s)?


================
Comment at: test/MC/RISCV/rvi-aliases-valid.s:179
+
+# CHECK-INST: srli a2, a3, 4
+# CHECK-ALIAS: srli a2, a3, 4
----------------
kito-cheng wrote:
> sabuasal wrote:
> > This test should print the canonical version of the instruction, you are printing the Alias. 
> Those alias instructions always disassemble to **i** version in GAS even -M no-aliases is given, my thought is keep the behavior with GAS.
Yes, we should match gas here. Perhaps add a comment to this test like "The following aliases are accepted as input but the canonical form of the instruction will always be printed."


https://reviews.llvm.org/D50046