[PATCH] D23566: [RISCV 8/10] Add support for all RV32I instructions

[James Y Knight via llvm-commits]

jyknight added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:162
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
----------------
theraven wrote:
> This probably wants to be `SImmScaled<12,1>(getConstantImm())`.
> 
> It would also be nice to use the same terminology (scaled immediate) as other back ends and the generic code, rather than simm-mask.
(This continues the thread from D23561, but it's more relevant here.)

I find it confusing to talk about the meanings as a transformation. I think the name ought to describe the value itself -- it *is* a 20 bit value, whose meaning is shifted by 1 bit. 

Thus, the convention that makes the most sense to me would be to call it an "sImm20s1" -- that is, 20 bits, shifted 1. This is the convention that the arm and aarch64 backends use.

Scaled is an okay word too, except that you might think it's a multiplier, not a shift. (does "scaled2" mean "times 2" or "shifted by 2"?)

Adding "left" or "right" into the name (as in "lsl" or "asr") is seems to me to be unnecessary clarification, that actually adds confusion instead of clarifying. When you specifies direction, I start thinking about what that's trying to say, and about which direction is which, and such. But there's only one sensible direction in the first place, so not saying it somewhat unintuitively seems to be less confusing -- at least for me.

(BTW, since the Aarch64 backend is a pretty new backend, and was written by many of the long-time core contributors to LLVM, I tend to look at it to guide style in preference to other backends. Of course it's not 100% the case that it's always doing things the best way, but I think it's probably more likely than others at the moment.)


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:156
 
+def FENCE : RISCVInst<(outs), (ins uimm4:$pred, uimm4:$succ), "fence\t$pred, $succ", []>
+{
----------------
This looks like it's actually an "FI" format instruction. I suggest the following:

```
def FENCE : FI<0b000, 0b0001111, (outs), (ins uimm4:$pred, uimm4:$succ), "fence\t$pred, $succ", []>
{   
  bits<4> pred;
  bits<4> succ;  
  
  let rs1 = 0;
  let rd = 0;  
  let imm12 = {0b0000,pred,succ};  
}

```


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:168
+
+def FENCEI : RISCVInst<(outs), (ins), "fence.i", []> {
+  let Opcode = 0b0001111;
----------------
```
def FENCEI : FI<0b001, 0b0001111, (outs), (ins), "fence.i", []> {
  let rs1 = 0;
  let rd = 0;
  let imm12 = 0;
}

```


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:190
+
+def CSRRW : CSR_rr<0b001, "csrrw">;
+def CSRRS : CSR_rr<0b010, "csrrs">;
----------------
Missing the csrr and csrw aliases.


================
Comment at: test/MC/RISCV/rv32i-valid.s:118
+
+# TODO: gnu assembler supports fence with no arguments
+fence 0, 15 # CHECK: encoding: [0x0f,0x00,0xf0,0x00]
----------------
This can be supported easily via adding:

```
def : InstAlias<"fence", (FENCE 0, 15)>;
```

(That also makes disassembly of "fence 0, 15" show up as "fence", automatically.


https://reviews.llvm.org/D23566