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
