[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);
+  }
----------------
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`?


https://reviews.llvm.org/D23566