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