zcmt table generation results in incorrect branch resolution

[simonpcook]

(This bug was originally found by a downstream user of the zcmt changes, but has been replicated with the CORE-V tools)

Generation of the Zcmt table appears to be resulting in branches being incorrectly resolved (amongst other corruptions).

Using embench-1.0 and my CORE-V toolchain build using these tools, if I build embench with:

./build_all.py --clean --arch riscv32 --board ri5cyverilator --cc riscv32-corev-elf-gcc --chip generic --cflags '-save-temps -c -march=rv32im_zcmt -mabi=ilp32 -mcmodel=medany -static -O2' --ldflags '-march=rv32im_zcmt -mabi=ilp32 -mcmodel=medany -static -nostdlib -nostartfiles'  --user-libs '-lm_nano -lc_nano -lgcc' --verbose

and disassemble nsichneu, looking for the first use of cm.jt, I find the following:

   11b9e:       01cf0463                beq     t5,t3,11ba6 <benchmark_body+0x1af0>
   11ba2:       a062                    cm.jt   24
   11ba4:       02872e03                lw      t3,40(a4)
   11ba8:       03472f03                lw      t5,52(a4)

Noting the branch target of that first branch, 11ba6 is in the middle of the first lw instruction quoted, so cannot be where we are expecting to branch to.

[jeremybennett]

@linsinan1995 @pz9115 Is this something you could look at?

[pz9115]

Yes, We will take a look at this problem.

[linsinan1995]

Thanks for pointing out this. The issue is introduced in the assembler where gas tries to relax a conditional branch by flipping the condition bit and setting the offset to a two-instruction-length constant (https://github.com/openhwgroup/corev-binutils-gdb/blob/development/gas/config/tc-riscv.c#L5007-L5022).

relax
    bne	t5,t3,.Lfar_away
to
    beq	t5,t3,8 # a constant value
    j .Lfar_away

Since beq is using a constant, it does not be handled by riscv_relax_delete_bytes during relaxation. To solve this issue, we could replace the destination of beq with a new label instead of a constant. I have some ongoing work on zcmt and can submit a fix later this week.

[linsinan1995]

btw, I thought the branch relaxation is a rare case, but it looks quite frequently when I look at the assembly code. The linker is able to convert beq + j back to bne if the range of bne is valid, which is the more ideal solution for both size and performance(reduce an unconditional jump). However, this part is missing in the ld riscv backend.

[linsinan1995]

Hi @simonpcook. It is expected to be fixed by #56, can you have a look at it?

[silabs-hfegran]

I tried the latest build with these fixes included as I am working on some zcmt-tests; and I noticed that certain jump instructions are replaced by cm.jt/cm.jalt without the table being generated. With --zcmt-force-table-jump it appears to work correctly.

[jeremybennett]

@silabs-hfegran Do you have a specific reproducer, so we can dig into this?

[silabs-hfegran]

@silabs-hfegran Do you have a specific reproducer, so we can dig into this?

@jeremybennett Sorry for the delayed response, I had some issues reproducing this without pulling in a lot of core-v-verif code, but this hello world example should exhibit the issue I mentioned:

https://github.com/silabs-hfegran/debug/tree/hf_dev_zcmt_issue
Run make all with the corev-openhw-gcc-centos7-20221213 toolchain and you should have a binary file that have tablejump instructions generated without generating the actual table

[silabs-hfegran]

Additionally, the naming for the tablejump section appears to be changed, as well as its location
(details in this PR: riscv-non-isa/riscv-elf-psabi-doc#349 )
.text.tbljal -> .riscv.jvt

[abukharmeh]

Yes it would be really really helpful if we can update to reflect the changes in riscv-non-isa/riscv-elf-psabi-doc#349 as that's currently the last item before ratification.

[linsinan1995]

@silabs-hfegran Do you have a specific reproducer, so we can dig into this?

@jeremybennett Sorry for the delayed response, I had some issues reproducing this without pulling in a lot of core-v-verif code, but this hello world example should exhibit the issue I mentioned:

https://github.com/silabs-hfegran/debug/tree/hf_dev_zcmt_issue Run make all with the corev-openhw-gcc-centos7-20221213 toolchain and you should have a binary file that have tablejump instructions generated without generating the actual table

Thanks for the feedback. I will take a look at it after my vacation. the test cases related to ZCMT are very inadequate, and this is something We need to work on afterwards.

Yes it would be really really helpful if we can update to reflect the changes in riscv-non-isa/riscv-elf-psabi-doc#349 as that's currently the last item before freezing.

The current ZC* version in corev toolchain is 0.70, so I think we can update the implementation to 1.0.0 in the meantime. There are not supposed to be many changes between these two versions. CC @pz9115

[jeremybennett]

This is fixed.