On 6/4/25 04:45, Richard Sandiford wrote:
>>>>> I think the issue is that:
>>>>>
>>>>> (insn 9 8 27 2 (parallel [
>>>>> (asm_operands/v ("fsrm %0") ("") 0 [
>>>>> (reg:SI 15 a5 [139])
>>>>> ]
>>>>> [
>>>>> (asm_input:SI ("r") frm-run-1.c:33)
>>>>> ]
>>>>> [] frm-run-1.c:33)
>>>>> (clobber (reg:V4096QI 69 frm))
>>>>> ]) "frm-run-1.c":33:3 -1
>>>>> (nil))
>>>>>
>>>>> is seen as invalidating FRM and so:
>>>>>
>>>>> (insn 27 9 28 2 (set (reg:SI 15 a5 [144])
>>>>> (reg:SI 69 frm)) "frm-run-1.c":43:1 2829 {frrmsi}
>>>>> (nil))
>>>>>
>>>>> is seen as an uninitialised read. I suppose clobbers in inline asms
>>>>> need to be treated as real definitions rather than just kills.
>>>> In general or specifically inside of late_combine ?
>>> We can start with the routines that rtl-ssa uses for its dataflow analysis.
>>> Does the patch below help?
>> Sorry, it doesn't. It seems the new code is not getting hit because @has_asm
>> is
>> false.
>> I tried a little hack to force it for my use case but it doesn't doesn't hit
>> the
>> new code.
>>
>> @@ -2236,7 +2236,7 @@ rtx_properties::try_to_add_src (const_rtx x, unsigned
>> int
>> flags)
>> }
>> else if (code == UNSPEC_VOLATILE)
>> has_volatile_refs = true;
>> - else if (code == ASM_INPUT || code == ASM_OPERANDS)
>> + else if (code == ASM_INPUT || code == ASM_OPERANDS || code == CLOBBER)
>>
>> Next I tried another mindless hack
>>
>> @@ -2295,7 +2295,7 @@ rtx_properties::try_to_add_pattern (const_rtx pat)
>> case CLOBBER:
>> {
>> rtx x = XEXP (pat, 0);
>> - if (has_asm && asm_clobber_is_read_modify_write_p (x))
>> + if (asm_clobber_is_read_modify_write_p (x))
>>
>> Now the new code hits, but the final outcome is still the same,
>> late_combine2 is
>> still eliminating those.
> Are you sure? Removing the "has_asm &&" works for me on your
> frm-mode-switch/upstream-v2 branch.
Sorry yes indeed the hack worked, I must have mixed up the dumpbase when
generating different outputs.
> But I think the non-hacky fix for the has_asm problem is the one below.
Thx, yes this works for my testcase as well.
Will you be pushing these 2 changes, the reference PR (for discussions at least)
could be 120245 and/or 120404.
Thx,
-Vineet
> Richard
>
>
> rtx_properties::try_to_add_pattern had a special case for
> ASM_OPERANDS. But this was a mistake, since:
>
> (a) it subverted the handling of ASM_OPERANDS in try_to_add_src.
>
> (b) it meant that there was an unnecessary divergence between the
> handling of asms with output operands and those without.
>
> I think this was probably something that I added early on,
> based on patterns elsewhere in the file, and didn't revisit later
> when the patch evolved.
>
> gcc/
> * rtlanal.cc (rtx_properties::try_to_add_pattern): Remove special
> handling of ASM_OPERANDS and defer to try_to_add_src instead.
> ---
> gcc/rtlanal.cc | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
> index 239d6691c4c..91cefae69be 100644
> --- a/gcc/rtlanal.cc
> +++ b/gcc/rtlanal.cc
> @@ -2256,11 +2256,6 @@ rtx_properties::try_to_add_pattern (const_rtx pat)
> break;
> }
>
> - case ASM_OPERANDS:
> - for (int i = 0, len = ASM_OPERANDS_INPUT_LENGTH (pat); i < len; ++i)
> - try_to_add_src (ASM_OPERANDS_INPUT (pat, i));
> - break;
> -
> case CLOBBER:
> try_to_add_dest (XEXP (pat, 0), rtx_obj_flags::IS_CLOBBER);
> break;
> @@ -2274,6 +2269,8 @@ rtx_properties::try_to_add_pattern (const_rtx pat)
> /* All the other possibilities never store and can use a normal
> rtx walk. This includes:
>
> + - ASM_INPUT
> + - ASM_OPERANDS
> - USE
> - TRAP_IF
> - PREFETCH
