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

[Alex Bradbury via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:162
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
----------------
asb wrote:
> 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.
> MIPS uses the SImmScaled functions, but naming is of the form `simm19_lsl2`. Both the lsl naming and the {U,I}ImmScaled functions are unique to MIPS currently so there's not broad consensus here - though I agree unifying terminology is useful.
> 
> I feel the naming is perhaps slightly confusing in that the decision to describe a transformation from the encoded to the 'actual' value seems arbitrary vs describing the transformation from 'actual' value to encoded value. The options I considered were:
> 
> * `simm20_lsl1:$imm20` (describes how to go from encoded value to logical value. Matches MIPS)
> * `simm21_asr1:$imm20` (describes going from logical value to encoded value)
> * `simm21_mask1:$imm20` (current approach, describes the constraints on the encoded value)
> 
> Are you suggesting `simm20_scaled1:$imm20`? Or perhaps `simm21_scaled1:$imm20`?
Or another alternative: given that in the RISC-V ISA the 'scaled' immediates only shift by 1 bit (UJ and SB instruction forms) we could go with `simm21_lsb0:$imm20` to indicate that the least significant bit is known 0.


https://reviews.llvm.org/D23566