On 10/12/18 10:30 AM, Bastian Koppelmann wrote:
> +static bool trans_c_addi(DisasContext *ctx, arg_c_addi *a, uint16_t insn)
> +{
> + if (a->imm == 0) {
> + return true;
> + }
return false, I think.
> +static bool trans_c_jal_addiw(DisasContext *ctx, arg_c_jal_addiw *a,
> + uint16_t insn)
> +{
> +#ifdef TARGET_RISCV32
> + /* C.JAL */
> + arg_c_j *tmp = g_new0(arg_c_j, 1);
> + extract_cj(tmp, insn);
Again with the g_new0 without free, which should use stack.
> + arg_jal arg = { .rd = 1, .imm = a->imm };
> + return trans_jal(ctx, &arg, insn);
> +#else
> + /* C.ADDIW */
> + arg_addiw arg = { .rd = a->rd, .rs1 = a->rd, .imm = a->imm };
> + return trans_addiw(ctx, &arg, insn);
> +#endif
> +}
> +
> +static bool trans_c_li(DisasContext *ctx, arg_c_li *a, uint16_t insn)
> +{
> + if (a->rd == 0) {
> + return true;
> + }
return false.
> +static bool trans_c_addi16sp_lui(DisasContext *ctx, arg_c_addi16sp_lui *a,
> + uint16_t insn)
> +{
> + if (a->rd == 2) {
> + /* C.ADDI16SP */
> + arg_addi arg = { .rd = 2, .rs1 = 2, .imm = a->imm_addi16sp };
> + return trans_addi(ctx, &arg, insn);
> + } else if (a->imm_lui != 0) {
> + if (a->rd == 0) {
> + return true;
> + }
I think it should be
} else if (a->imm_lui != 0 && a->rd != 0) {
> + /* C.LUI */
> + arg_lui arg = { .rd = a->rd, .imm = a->imm_lui };
> + return trans_lui(ctx, &arg, insn);
> + }
> + return false;
> +}
> +
> +static bool trans_c_srli(DisasContext *ctx, arg_c_srli *a, uint16_t insn)
> +{
> + if (a->shamt == 0) {
> + /* Reserved in ISA */
> + gen_exception_illegal(ctx);
> + return true;
> + }
Choose return false or raise exception. Except...
I wonder if we might write this as
int shamt = a->shamt;
if (shamt == 0) {
shamt = 64;
}
> +#ifdef TARGET_RISCV32
> + /* Ensure, that shamt[5] is zero for RV32 */
> + if (a->shamt >= 32) {
> + gen_exception_illegal(ctx);
> + return true;
> + }
> +#endif
then this is unconditional as
if (a->shamt >= TARGET_LONG_BITS)
which makes it clear that "reserved in isa" already has a meaning.
> +
> + arg_srli arg = { .rd = a->rd, .rs1 = a->rd, .shamt = a->shamt };
> + return trans_srli(ctx, &arg, insn);
Although I do wonder about moving that check into trans_srli et al, rather than
bend over backward parsing 6-bit shift amount rather just using @i format.
r~
