Re: [PATCH v5] RISCV: Inline subword atomic ops

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/18/23 14:48, Patrick O'Neill wrote:
> On 4/18/23 09:59, Jeff Law wrote:
>> On 4/18/23 08:28, Patrick O'Neill wrote:
>> ...
>>> +  rtx addr = force_reg (Pmode, XEXP (mem, 0));
>>> +
>>> +  rtx aligned_addr = gen_reg_rtx (Pmode);
>>> +  emit_move_insn (aligned_addr,  gen_rtx_AND (Pmode, addr,
>>> +                          gen_int_mode (-4, Pmode)));
>> So rather than -4 as a magic number, GET_MODE_MASK would be better. 
>> That may result in needing to rewrap this code.  I'd bring the 
>> gen_rtx_AND down on a new line, aligned with aligned_addr.
> IIUC GET_MODE_MASK generates masks like 0xFF for QI (for example). It 
> doesn't have the granularity to generate 0x3 (which we can NOT to get 
> -4). I searched the GCC internals docs but couldn't find a function that 
> does address alignment masks.
Yea, yea.  Big "duh" on my side.

>> Presumably using SImode is intentional here rather than wanting to use 
>> word_mode which would be SImode for rv32 and DImode for rv64?  I'm 
>> going to work based on that assumption, but if it isn't there's more 
>> work to do to generalize this code.
> It's been a year but IIRC it was just simpler to implement (and to me it 
> didn't make sense to use 64 bits for a subword op).
> Is there a benefit in using 64 bit instructions when computing subwords?
Given that rv64 should have 32bit load/stores, I don't offhand see any 
advantage.


>>> +
>>> +(define_expand "atomic_fetch_nand<mode>"
>>> +  [(set (match_operand:SHORT 0 "register_operand" "=&r")
>>> +    (match_operand:SHORT 1 "memory_operand" "+A"))
>>> +   (set (match_dup 1)
>>> +    (unspec_volatile:SHORT
>>> +      [(not:SHORT (and:SHORT (match_dup 1)
>>> +                 (match_operand:SHORT 2 "reg_or_0_operand" "rJ")))
>>> +       (match_operand:SI 3 "const_int_operand")] ;; model
>>> +     UNSPEC_SYNC_OLD_OP_SUBWORD))]
>>> +  "TARGET_ATOMIC && TARGET_INLINE_SUBWORD_ATOMIC"
>> Just a note, constraints aren't necessary for a define_expand. They 
>> don't hurt anything though.  They do document expectations, but then 
>> you have to maintain them over time.  I'm OK leaving them, mostly 
>> wanted to make sure you're aware they aren't strictly necessary for a 
>> define_expand.
> I wasn't aware, thanks for pointing it out! - you're referring to the 
> "TARGET_ATOMIC && TARGET_INLINE_SUBWORD_ATOMIC", (not the register 
> constraints) right?
I was referring to the register constraints like "=&r".  They're ignored 
on define_expand constructors.  A define_expand generates RTL that will 
be matched later by a define_insn.

The "TARGET_ATOMIC && TARGET_INLINE_SUBWORD_ATOMIC" is usually referred 
to as the insn condition.


> Thanks for reviewing!
NP.  Looking forward to V6 which I expect will be ready for inclusion.

jeff