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

David Chisnall via llvm-commits llvm-commits at lists.llvm.org
Sat Oct 8 03:03:05 PDT 2016


theraven added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:162
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
----------------
asb wrote:
> 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.
I'm happy with either name, as long as there's a comment explaining what it means where it's first introduced.  I'm more concerned to avoid the reimplementation of `SImmScaled` than what you call the result.


https://reviews.llvm.org/D23566





More information about the llvm-commits mailing list