| Message ID | 20230519021926.15362-4-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for extension specific disas | expand |
On 5/18/23 23:19, Weiwei Li wrote: > Support disas for Zcmt* instructions only when related extensions > are supported. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > disas/riscv.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/disas/riscv.c b/disas/riscv.c > index 729ab684da..9e01810eef 100644 > --- a/disas/riscv.c > +++ b/disas/riscv.c > @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) > op = rv_op_c_sqsp; > } else { > op = rv_op_c_fsdsp; > - if (((inst >> 12) & 0b01)) { > + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { > switch ((inst >> 8) & 0b01111) { > case 8: > if (((inst >> 4) & 0b01111) >= 4) { > @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) > } else { > switch ((inst >> 10) & 0b011) { > case 0: > - if (((inst >> 2) & 0xFF) >= 32) { > - op = rv_op_cm_jalt; > - } else { > - op = rv_op_cm_jt; > + if (dec->cfg->ext_zcmt) { > + if (((inst >> 2) & 0xFF) >= 32) { > + op = rv_op_cm_jalt; > + } else { > + op = rv_op_cm_jt; > + } In this code, since you're not doing anything if dec->cfg->ext_zcmt is not set, you could also break earlier and avoid changing the other lines: > case 0: > + if (!dec->cfg->ext_zcmt) { > + break; > + } > if (((inst >> 2) & 0xFF) >= 32) { > op = rv_op_cm_jalt; > } else { > op = rv_op_cm_jt; > } > break; > } > break; > case 3: > - switch ((inst >> 5) & 0b011) { > - case 1: op = rv_op_cm_mvsa01; break; > - case 3: op = rv_op_cm_mva01s; break; > + if (dec->cfg->ext_zcmp) { > + switch ((inst >> 5) & 0b011) { > + case 1: op = rv_op_cm_mvsa01; break; > + case 3: op = rv_op_cm_mva01s; break; > + } Same thing here. These are minor stylistic comments. Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > } > break; > }
In fact, apparently checkpatch.pl is not too happy about this patch: On 5/18/23 23:19, Weiwei Li wrote: > Support disas for Zcmt* instructions only when related extensions > are supported. > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> > --- > disas/riscv.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/disas/riscv.c b/disas/riscv.c > index 729ab684da..9e01810eef 100644 > --- a/disas/riscv.c > +++ b/disas/riscv.c > @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) > op = rv_op_c_sqsp; > } else { > op = rv_op_c_fsdsp; > - if (((inst >> 12) & 0b01)) { > + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { > switch ((inst >> 8) & 0b01111) { > case 8: > if (((inst >> 4) & 0b01111) >= 4) { > @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) > } else { > switch ((inst >> 10) & 0b011) { > case 0: > - if (((inst >> 2) & 0xFF) >= 32) { > - op = rv_op_cm_jalt; > - } else { > - op = rv_op_cm_jt; > + if (dec->cfg->ext_zcmt) { > + if (((inst >> 2) & 0xFF) >= 32) { > + op = rv_op_cm_jalt; > + } else { > + op = rv_op_cm_jt; > + } > } > break; > case 3: > - switch ((inst >> 5) & 0b011) { > - case 1: op = rv_op_cm_mvsa01; break; > - case 3: op = rv_op_cm_mva01s; break; > + if (dec->cfg->ext_zcmp) { > + switch ((inst >> 5) & 0b011) { > + case 1: op = rv_op_cm_mvsa01; break; > + case 3: op = rv_op_cm_mva01s; break; > + } At this point: ================ 3/7 Checking commit 989059d476f9 (disas/riscv.c: Support disas for Zcm* extensions) ERROR: trailing statements should be on next line #51: FILE: disas/riscv.c:2541: + case 1: op = rv_op_cm_mvsa01; break; ERROR: trailing statements should be on next line #52: FILE: disas/riscv.c:2542: + case 3: op = rv_op_cm_mva01s; break; total: 2 errors, 0 warnings, 35 lines checked Patch 3/7 has style problems, please review. If any of these errors are false positives report them to the maintainer, see ================ The issue predates your patch. It would be very nice of you if you can fix it though :D Daniel > } > break; > }
On 2023/5/22 21:00, Daniel Henrique Barboza wrote: > > > On 5/18/23 23:19, Weiwei Li wrote: >> Support disas for Zcmt* instructions only when related extensions >> are supported. >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> disas/riscv.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/disas/riscv.c b/disas/riscv.c >> index 729ab684da..9e01810eef 100644 >> --- a/disas/riscv.c >> +++ b/disas/riscv.c >> @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, >> rv_isa isa) >> op = rv_op_c_sqsp; >> } else { >> op = rv_op_c_fsdsp; >> - if (((inst >> 12) & 0b01)) { >> + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { >> switch ((inst >> 8) & 0b01111) { >> case 8: >> if (((inst >> 4) & 0b01111) >= 4) { >> @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode >> *dec, rv_isa isa) >> } else { >> switch ((inst >> 10) & 0b011) { >> case 0: >> - if (((inst >> 2) & 0xFF) >= 32) { >> - op = rv_op_cm_jalt; >> - } else { >> - op = rv_op_cm_jt; >> + if (dec->cfg->ext_zcmt) { >> + if (((inst >> 2) & 0xFF) >= 32) { >> + op = rv_op_cm_jalt; >> + } else { >> + op = rv_op_cm_jt; >> + } > > In this code, since you're not doing anything if dec->cfg->ext_zcmt is > not set, > you could also break earlier and avoid changing the other lines: > > >> case 0: >> + if (!dec->cfg->ext_zcmt) { >> + break; >> + } >> if (((inst >> 2) & 0xFF) >= 32) { >> op = rv_op_cm_jalt; >> } else { >> op = rv_op_cm_jt; >> } >> break; > OK. It's acceptable to me. I'll update this in next version. Regards, Weiwei Li > > } >> break; >> case 3: >> - switch ((inst >> 5) & 0b011) { >> - case 1: op = rv_op_cm_mvsa01; break; >> - case 3: op = rv_op_cm_mva01s; break; >> + if (dec->cfg->ext_zcmp) { >> + switch ((inst >> 5) & 0b011) { >> + case 1: op = rv_op_cm_mvsa01; break; >> + case 3: op = rv_op_cm_mva01s; break; >> + } > > > Same thing here. > > > These are minor stylistic comments. > > > Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > >> } >> break; >> }
On 2023/5/22 21:10, Daniel Henrique Barboza wrote: > In fact, apparently checkpatch.pl is not too happy about this patch: > > On 5/18/23 23:19, Weiwei Li wrote: >> Support disas for Zcmt* instructions only when related extensions >> are supported. >> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >> --- >> disas/riscv.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/disas/riscv.c b/disas/riscv.c >> index 729ab684da..9e01810eef 100644 >> --- a/disas/riscv.c >> +++ b/disas/riscv.c >> @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, >> rv_isa isa) >> op = rv_op_c_sqsp; >> } else { >> op = rv_op_c_fsdsp; >> - if (((inst >> 12) & 0b01)) { >> + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { >> switch ((inst >> 8) & 0b01111) { >> case 8: >> if (((inst >> 4) & 0b01111) >= 4) { >> @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode >> *dec, rv_isa isa) >> } else { >> switch ((inst >> 10) & 0b011) { >> case 0: >> - if (((inst >> 2) & 0xFF) >= 32) { >> - op = rv_op_cm_jalt; >> - } else { >> - op = rv_op_cm_jt; >> + if (dec->cfg->ext_zcmt) { >> + if (((inst >> 2) & 0xFF) >= 32) { >> + op = rv_op_cm_jalt; >> + } else { >> + op = rv_op_cm_jt; >> + } >> } >> break; >> case 3: >> - switch ((inst >> 5) & 0b011) { >> - case 1: op = rv_op_cm_mvsa01; break; >> - case 3: op = rv_op_cm_mva01s; break; >> + if (dec->cfg->ext_zcmp) { >> + switch ((inst >> 5) & 0b011) { >> + case 1: op = rv_op_cm_mvsa01; break; >> + case 3: op = rv_op_cm_mva01s; break; >> + } > > At this point: > > ================ > 3/7 Checking commit 989059d476f9 (disas/riscv.c: Support disas for > Zcm* extensions) > ERROR: trailing statements should be on next line > #51: FILE: disas/riscv.c:2541: > + case 1: op = rv_op_cm_mvsa01; break; > > ERROR: trailing statements should be on next line > #52: FILE: disas/riscv.c:2542: > + case 3: op = rv_op_cm_mva01s; break; > > total: 2 errors, 0 warnings, 35 lines checked > > Patch 3/7 has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > ================ > Yeah. I also found this errors when I ran the checkpatch.pl. However, this is the usual code style in this file. So I didn't fix it. Regards, Weiwei Li > > The issue predates your patch. It would be very nice of you if you can > fix it > though :D > > > > Daniel > > >> } >> break; >> }
On 5/22/23 11:27, Weiwei Li wrote: > > On 2023/5/22 21:10, Daniel Henrique Barboza wrote: >> In fact, apparently checkpatch.pl is not too happy about this patch: >> >> On 5/18/23 23:19, Weiwei Li wrote: >>> Support disas for Zcmt* instructions only when related extensions >>> are supported. >>> >>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn> >>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn> >>> --- >>> disas/riscv.c | 20 ++++++++++++-------- >>> 1 file changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/disas/riscv.c b/disas/riscv.c >>> index 729ab684da..9e01810eef 100644 >>> --- a/disas/riscv.c >>> +++ b/disas/riscv.c >>> @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) >>> op = rv_op_c_sqsp; >>> } else { >>> op = rv_op_c_fsdsp; >>> - if (((inst >> 12) & 0b01)) { >>> + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { >>> switch ((inst >> 8) & 0b01111) { >>> case 8: >>> if (((inst >> 4) & 0b01111) >= 4) { >>> @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) >>> } else { >>> switch ((inst >> 10) & 0b011) { >>> case 0: >>> - if (((inst >> 2) & 0xFF) >= 32) { >>> - op = rv_op_cm_jalt; >>> - } else { >>> - op = rv_op_cm_jt; >>> + if (dec->cfg->ext_zcmt) { >>> + if (((inst >> 2) & 0xFF) >= 32) { >>> + op = rv_op_cm_jalt; >>> + } else { >>> + op = rv_op_cm_jt; >>> + } >>> } >>> break; >>> case 3: >>> - switch ((inst >> 5) & 0b011) { >>> - case 1: op = rv_op_cm_mvsa01; break; >>> - case 3: op = rv_op_cm_mva01s; break; >>> + if (dec->cfg->ext_zcmp) { >>> + switch ((inst >> 5) & 0b011) { >>> + case 1: op = rv_op_cm_mvsa01; break; >>> + case 3: op = rv_op_cm_mva01s; break; >>> + } >> >> At this point: >> >> ================ >> 3/7 Checking commit 989059d476f9 (disas/riscv.c: Support disas for Zcm* extensions) >> ERROR: trailing statements should be on next line >> #51: FILE: disas/riscv.c:2541: >> + case 1: op = rv_op_cm_mvsa01; break; >> >> ERROR: trailing statements should be on next line >> #52: FILE: disas/riscv.c:2542: >> + case 3: op = rv_op_cm_mva01s; break; >> >> total: 2 errors, 0 warnings, 35 lines checked >> >> Patch 3/7 has style problems, please review. If any of these errors >> are false positives report them to the maintainer, see >> ================ >> > Yeah. I also found this errors when I ran the checkpatch.pl. > > However, this is the usual code style in this file. So I didn't fix it. As long as the maintainer is ok with it (since it'll make noise in Gitlab when running the test runners and so on) we can leave it as is. Daniel > > Regards, > > Weiwei Li > >> >> The issue predates your patch. It would be very nice of you if you can fix it >> though :D >> >> >> >> Daniel >> >> >>> } >>> break; >>> } >
diff --git a/disas/riscv.c b/disas/riscv.c index 729ab684da..9e01810eef 100644 --- a/disas/riscv.c +++ b/disas/riscv.c @@ -2501,7 +2501,7 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) op = rv_op_c_sqsp; } else { op = rv_op_c_fsdsp; - if (((inst >> 12) & 0b01)) { + if (dec->cfg->ext_zcmp && ((inst >> 12) & 0b01)) { switch ((inst >> 8) & 0b01111) { case 8: if (((inst >> 4) & 0b01111) >= 4) { @@ -2527,16 +2527,20 @@ static void decode_inst_opcode(rv_decode *dec, rv_isa isa) } else { switch ((inst >> 10) & 0b011) { case 0: - if (((inst >> 2) & 0xFF) >= 32) { - op = rv_op_cm_jalt; - } else { - op = rv_op_cm_jt; + if (dec->cfg->ext_zcmt) { + if (((inst >> 2) & 0xFF) >= 32) { + op = rv_op_cm_jalt; + } else { + op = rv_op_cm_jt; + } } break; case 3: - switch ((inst >> 5) & 0b011) { - case 1: op = rv_op_cm_mvsa01; break; - case 3: op = rv_op_cm_mva01s; break; + if (dec->cfg->ext_zcmp) { + switch ((inst >> 5) & 0b011) { + case 1: op = rv_op_cm_mvsa01; break; + case 3: op = rv_op_cm_mva01s; break; + } } break; }