On Wed, Feb 19, 2025 at 12:53 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> Since stack can only be accessed by GPR, check GENERAL_REG_P, instead of
> REG_P, in ix86_find_all_reg_use_1.
>
> gcc/
>
> PR target/118936
> * config/i386/i386.cc (ix86_find_all_reg_use_1): Replace REG_P
> with GENERAL_REG_P.
>
> gcc/testsuite/
>
> PR target/118936
> * gcc.target/i386/pr118936.c: New test.
>
> OK for master?
As said in the PR, this change "fixes" only this particular problem,
where we have:
97: ax:DI='g_1680'
170: xmm0:DI=ax:DI
98: xmm0:V2DI=vec_duplicate(xmm0:DI)
REG_EQUIV vec_duplicate(`g_1680')
101: [`g_1679']=xmm0:V2DI
103: [const(`g_1679'+0x10)]=xmm0:V2DI
105: [const(`g_1679'+0x20)]=xmm0:V2DI
107: [const(`g_1679'+0x30)]=xmm0:V2DI
But does not fix the fact that your algorithm considers these stores
even when they are in no way connected to stack or frame pointer.
These do not even use registers in their address.
Previous approach did this:
- if (check_stack_slot)
- {
- /* Find the maximum stack alignment. */
- subrtx_iterator::array_type array;
- FOR_EACH_SUBRTX (iter, array, PATTERN (insn), ALL)
- if (MEM_P (*iter)
- && (reg_mentioned_p (stack_pointer_rtx,
- *iter)
- || reg_mentioned_p (frame_pointer_rtx,
- *iter)))
- {
- unsigned int alignment = MEM_ALIGN (*iter);
- if (alignment > stack_alignment)
- stack_alignment = alignment;
- }
- }
So, anywhere in the insn SP or FP was mentioned (either memory store
or load), the alignment was increased as requested by MEM. The issue
here is that another register that is calculated from SP or FP can be
used to access the stack slot. I'm not sure I know how your algorithm
works id detail, it detects e.g.:
103: [const(`g_1679'+0x10)]=xmm0:V2DI
as a candidate to increase the alignment, but its address doesn't even
use registers, let alone SP or FP. Also,
+ note_stores (insn, ix86_update_stack_alignment,
+ &stack_alignment);
in your algorithm does not consider that loads from the stack also
increase alignment requirements.
Can you please explain the approach in your original patch in some
more detail? I'd expect a variant of the original approach, where the
compiler keeps an up-to-date list of registers that depend on SP or
FP, and instead of a naive:
reg_mentioned_p (stack_pointer_rtx, *iter)
it goes through a list, formed of SP, FP and their dependent registers
and updates stack alignment if the insn memory address uses the
register on the list.
Uros.
