Andrew Stubbs <[email protected]> writes:
> On 09/10/2026 16:22, Richard Sandiford wrote:
>> Andrew Stubbs <[email protected]> writes:
>>> On 09/10/2026 12:15, Richard Sandiford wrote:
>>>> Sorry, a couple of other things:
>>>>
>>>>>> +
>>>>>> + rtx new_addr
>>>>>> + = (REG_P (addr)
>>>>>> + ? simplify_gen_subreg (addr_elt_mode, addr, addrmode,
>>>>>> + addr_byte)
>>>>>> + : simplify_subreg (addr_elt_mode, addr, addrmode,
>>>>>> addr_byte));
>>>>
>>>> I think we should use simplify_gen_subreg unconditionally unless there's
>>>> a specific reason not to. This is a simplification regardless of what
>>>> the address is.
>>>
>>> I tried this and got an ICE. AFAICT, simplify_gen_subreg does the wrong
>>> thing (in this situation) when the address has an offset:
>>>
>>> (mem/i:V8QI (plus:V8DI (reg:V8DI 720)
>>> (vec_duplicate:V8DI (const_int 0))))
>>>
>>> It tries to wrap the plus in a subreg, which is not valid. So, I'm going
>>> to leave this bit as-is, unless you have a better suggestion?
>>
>> Why's it not valid though?
> gcn_addr_space_legitimate_address_p rejects it because there's no
> instruction that can do that in a load/store instruction.
Yeah, I agree that it should do that.
> Is that not how that's supposed to work?
Yeah, gcn_addr_space_legitimate_address_p should describe the
target restrictions.
But simplification routines aren't required or expected to restrict
themselves to things that the target supports directly. It's up to the
caller of the simplification routines to test whether the target supports
a particular operation. E.g. elsewhere we have:
/* If we have a SUBREG of a register that we are replacing and we are
replacing it with a MEM, make a new MEM and try replacing the
SUBREG with it. Don't do this if the MEM has a mode-dependent address
or if we would be widening it. */
if (MEM_P (op)
&& ! mode_dependent_address_p (XEXP (op, 0), MEM_ADDR_SPACE (op))
/* Allow splitting of volatile memory references in case we don't
have instruction to move the whole thing. */
&& (! MEM_VOLATILE_P (op)
|| ! have_insn_for (SET, innermode))
&& !(STRICT_ALIGNMENT && MEM_ALIGN (op) < GET_MODE_ALIGNMENT (outermode))
&& known_le (outersize, innersize))
return adjust_address_nv (op, outermode, byte);
The mode_dependent_address_p check admittedly muddies the waters, but the
use of adjust_address_nv means that the new MEM isn't necessarily valid.
> There's also a chance to fix it in gcn_addr_space_legitimize_address, so
> I'm trying to figure out if that's something it should be doing.
>
> The testcase is trying to convert
>
> (mem/i:V8QI (plus:V8DI (reg:V8DI 720)
> (vec_duplicate:V8DI (const_int 0))))
>
> into
>
> (mem:QI (subreg:DI (plus:V8DI (reg:V8DI 720)
> (vec_duplicate:V8DI (const_int 0)))
> 0))
>
> Which is because CSE is trying to split this
>
> (set (mem/i:V8QI (plus:V8DI (reg:V8DI 720)
> (vec_duplicate:V8DI (const_int 0))))
> (const_vector:V8QI [(const_int 0 [0]) repeated x8]))
>
> into 8 separate operations. The comment says "These are templates and
> don't actually get emitted but are used to tell CSE how to get to a
> particular constant."
That too sounds ok. But CSE should only use the registered MEMs in an
insn if the insn passes a recog test.
Do you have a backtrace for the ICE?
Thanks,
Richard
