Re: [PATCH 4/5] RISC-V: Prefetch hint instructions and operand set

[Nelson Chu <nelson.chu@sifive.com>]

Hi Tsukasa,

Yeah you are right, we should define a new operand for the imm of
prefetch instructions, to check if the lower 5 bit is zero.  Sorry
that I thought wrong before.  The patches in the riscv-CMOs-with-paren
branch from your github LGTM, so sommited.

Thanks
Nelson

On Fri, Feb 25, 2022 at 3:07 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote:
>
> Hi Nelson,
>
> I attached new rebased version of my CMO patches (for reference and
> NOT an actual submission) so that you can test an example below.
>
> | (Submission)   (Attachment)
> | PATCH 1/5   -> PATCH 1-2/2 (splitted and squashed)
> | PATCH 4-5/5 -> PATCH 1/2 (squashed and order changed) p
> | PATCH 2-3/5 -> PATCH 2/2 (squashed and order changed) m/z
>
> This reorganization is mainly caused by following open issue:
> <https://github.com/riscv/riscv-CMOs/issues/47>
> I hope this is resolved soon so that I can submit the next version
> without extra footnotes.
>
>
> On 2022/02/25 11:50, Nelson Chu wrote:
> > Hi Tsukasa,
> >
> > Reusing the q operand should be enough.  Though we may add relocation
> > when the offset is a symbol, I think this should be fine.
> >
> > Consider Christoph patch,
> > https://sourceware.org/pipermail/binutils/2022-January/119262.html
> >
> > His patch is closer to the implementation I will do, just consider the
> > compatibility of amo instructions should be perfect.  However, you are
> > adding more testcases than us.  It would be always good to add more
> > testcases, so except the f operand, your patches LGTM.
> >
> > Thanks
> > Nelson
>
>
> Unfortunately, simply reusing 'q' doesn't work.
>
> For overflow checking, 'q' is fine.
>
> On 2022/02/25 11:50, Nelson Chu wrote:>>         case 'q': used_bits |= ENCODE_STYPE_IMM (-1U); break;
> >> +       case 'f': used_bits |= ENCODE_STYPE_IMM (-1U); break;
> >
> > I prefer to reuse the q operand here.
>
> We may use simple fall-through here.
>
> However, 32-byte alignment constraint requires a new operand type.
>
>
> Let's see... assume that we change to use "q(s)" instead of "f(s)" (you
> can just patch the patches I attached).
>
> # BTW, I use minimal playground to test my GNU Binutils patches
> # <https://github.com/a4lg/riscv-baremetal-playground>
> # so that this example can be built and linked.
>     .globl  _start
>     .section .entry, "ax", %progbits
> _start:
>     .option arch,+zicbop
>     prefetch.i 0(t0)
>     # Remove Zicbop extension from final ELF
>     # to disassemble above instruction as `ori', not `prefetch.i'.
>     .option arch,-zicbop
>     ret
>
> If we assemble above example (test.bare.S) and disassemble the result,
> it should look like this:
>
> 0000000080000028 <_start>:
>     80000028:   0002e013          ori  zero,t0,0
>     8000002c:   8082              ret
>
> Looks great.  But if we change 0(t0) to 1(t0), we DON'T get an error and
> the result will look like...
>
> 0000000080000028 <_start>:
>     80000028:   0002e093          ori  ra,t0,0
>     8000002c:   8082              ret
>
> Oh no, a side effect (assignment to ra[x1]) without "illegal operands".
>
>
> It happens because `match_opcode' for prefetch instructions (to test
> instruction validity in `riscv_ip') is called before instruction bits
> are modified.  For store addresses, actual value insertion happens in
> `md_apply_fix' function after "successfully assembled" in `riscv_ip'.
>
> So, we need to test validity of the immediate constant (32-byte aligned)
> somewhere.  Even if we test it on fixup, it (at least) requires new
> BFD relocation type (preventing reuse of case 'q' code path).
>
>
> Of course, if we consider using relocations on prefetch instructions,
> we could move, for instance, address insertion.
> Still, we absolutely need something new to handle special 32-byte
> alignment or we will risk generating corrupted programs.
>
>
> Thanks,
> Tsukasa