[i386] Recompute the frame layout less often

On 05/15/2017 03:39 PM, Bernd Edlinger wrote:
> On 05/15/17 03:39, Daniel Santos wrote:
>> On 05/14/2017 11:31 AM, Bernd Edlinger wrote:
>>> Hi Daniel,
>>>
>>> there is one thing I don't understand in your patch:
>>> That is, it introduces a static value:
>>>
>>> /* Registers who's save & restore will be managed by stubs called from
>>>       pro/epilogue.  */
>>> static HARD_REG_SET GTY(()) stub_managed_regs;
>>>
>>> This seems to be set as a side effect of ix86_compute_frame_layout,
>>> and depends on the register usage of the current function.
>>> But values that depend on the current function need usually be
>>> attached to cfun->machine, because the passes can run in parallel
>>> unless I am completely mistaken, and the stub_managed_regs may
>>> therefore be computed from a different function.
>>>
>>>
>>> Bernd.
>> I should add that if you want to run faster tests just on the ms to sysv
>> abi code, you can use make RUNTESTFLAGS="ms-sysv.exp" check and then if
>> that succeeds run the full testsuite.
>>
>> Daniel
> Unfortunately I encounter a serious problem when my patch is used
> ontop of your patch, Yes, the test suite ran without error, but then
> I tried to trigger the warning and that tripped an ICE.
> The reason is that cfun->machine->call_ms2sysv can be set to true
> *after* reload_completed, which can be seen using the following
> patch:
>
> Index: i386.c
> ===================================================================
> --- i386.c	(revision 248031)
> +++ i386.c	(working copy)
> @@ -29320,7 +29320,10 @@
>
>          /* Set here, but it may get cleared later.  */
>          if (TARGET_CALL_MS2SYSV_XLOGUES)
> +      {
> +	gcc_assert(!reload_completed);
>    	cfun->machine->call_ms2sysv = true;
> +      }
>        }
>
>      if (vec_len > 1)
>
>
> That assertion is triggered in this test case:
>
> cat test.c
> int test()
> {
>     __builtin_printf("test\n");
>     return 0;
> }
>
> gcc -mabi=ms -mcall-ms2sysv-xlogues -fsplit-stack -c test.c
> test.c: In function 'test':
> test.c:5:1: internal compiler error: in ix86_expand_call, at
> config/i386/i386.c:29324
>    }
>    ^
> 0x13390a4 ix86_expand_call(rtx_def*, rtx_def*, rtx_def*, rtx_def*,
> rtx_def*, bool)
> 	../../gcc-trunk/gcc/config/i386/i386.c:29324
> 0x1317494 ix86_expand_split_stack_prologue()
> 	../../gcc-trunk/gcc/config/i386/i386.c:15920
> 0x162ba21 gen_split_stack_prologue()
> 	../../gcc-trunk/gcc/config/i386/i386.md:12556
> 0x12f3f30 target_gen_split_stack_prologue
> 	../../gcc-trunk/gcc/config/i386/i386.md:12325
> 0xb237b3 make_split_prologue_seq
> 	../../gcc-trunk/gcc/function.c:5822
> 0xb23a08 thread_prologue_and_epilogue_insns()
> 	../../gcc-trunk/gcc/function.c:5958
> 0xb24840 rest_of_handle_thread_prologue_and_epilogue
> 	../../gcc-trunk/gcc/function.c:6428
> 0xb248c0 execute
> 	../../gcc-trunk/gcc/function.c:6470
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <https://gcc.gnu.org/bugs/> for instructions.
>
>
> so, in ix86_expand_split_stack_prologue
> we first call:
>     ix86_finalize_stack_realign_flags ();
>     ix86_compute_frame_layout (&frame);
>
> and later:
>     call_insn = ix86_expand_call (NULL_RTX, gen_rtx_MEM (QImode, fn),
>                                   GEN_INT (UNITS_PER_WORD), constm1_rtx,
>                                   pop, false);
>
> which changes a flag with a huge impact on the frame layout, but there
> is no absolutely no way how the frame layout can change once it is
> finalized.
>
>
> Any Thoughts?
>
>
> Bernd.

Well, my intention was actually to punt on those cases, but I hadn't 
actually tested with -fsplit-stack.  It looks like 
ix86_expand_split_stack_prologue calls ix86_expand_call, and I hadn't 
anticipated it getting called after the last call to 
ix86_compute_frame_layout(), which your patch has probably eliminated.  
In the case of -fsplit-stack, I'm testing the macro flag_split_stack 
which (currently) just expands to check the global flag, so this could 
instead be done in ix86_option_override_internal () instead, but I think 
it highlights a somewhat deeper problem.

Rather or not m->call_ms2sysv is set determines which stack layout is 
used when ix86_compute_frame_layout() runs.  But if we can run 
expand_call after the final time ix86_compute_frame_layout() then we 
have a problem.  It looks like ix86_expand_split_stack_prologue is the 
only function that manually calls ix86_expand_call, but maybe it would 
be better to modify the test to something like this:


Or even use the same incompatibility tests from 
ix86_compute_frame_layout (and possibly move them to a static).

But I didn't get an ICE when using a build from a few days ago. I've 
updated to the current HEAD but I'm having problems with the test 
failing even though there's no errors, as if expect has some finite 
buffer for all of the warning spam and it kills the job.  I'm running 
another test to try and figure that out, but I have to run, so I'll get 
back with you on the results.

Daniel

Ian, would you mind looking at this please?  A combination of my 
-mcall-ms2sysv-xlogues patch with Bernd's patch is causing problems when 
ix86_expand_split_stack_prologue() calls ix86_expand_call().

On 05/15/2017 06:46 PM, Daniel Santos wrote:
> Rather or not m->call_ms2sysv is set determines which stack layout is 
> used when ix86_compute_frame_layout() runs. But if we can run 
> expand_call after the final time ix86_compute_frame_layout() then we 
> have a problem.  It looks like ix86_expand_split_stack_prologue is the 
> only function that manually calls ix86_expand_call, but maybe it would 
> be better to modify the test to something like this:
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index a78819d6b3f..c36383f6962 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -29325,7 +29325,7 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx 
> callarg1,
>         }
>
>        /* Set here, but it may get cleared later.  */
> -      if (TARGET_CALL_MS2SYSV_XLOGUES)
> +      if (TARGET_CALL_MS2SYSV_XLOGUES && !reload_completed)
>         cfun->machine->call_ms2sysv = true;
>      }
>
>

Actually, I think this is wrong.  I happened to recall looking at the 
morestack code last year and remembered that it was all assembly.  I 
looked at it again and I don't see that it calls anything outside of 
it's implementation file (libgcc/config/i386/morestack.S) except for 
_Unwind_Resume and the calling function its self (I think it calls its 
caller).  It saves and restores rsi and rdi and doesn't use any sse 
registers, so it doesn't need to clobber all of the regs in the 
x86_64_ms_sysv_extra_clobbered_registers array.  I'm guessing that this 
should have it's own pattern instead of calling ix86_expand_call in the 
first place.

Of course, I'm the new guy here, so please enlighten me if I'm wrong.

Thanks,
Daniel

On Mon, May 15, 2017 at 10:00 PM, Daniel Santos <daniel.santos@pobox.com> wrote:
>
> Ian, would you mind looking at this please?  A combination of my
> -mcall-ms2sysv-xlogues patch with Bernd's patch is causing problems when
> ix86_expand_split_stack_prologue() calls ix86_expand_call().

I don't have a lot of context here.  I assume that ms2sysv is going to
be used on Windows systems, where -fsplit-stack isn't really going to
work anyhow, so I think it would probably be OK that reject that
combination if it causes trouble.

Also, it's overkill for ix86_expand_split_stack_prologue to call
ix86_expand_call.  The call is always to __morestack, and __morestack
is written in assembler, so we could use a simpler version of
ix86_expand_call if that helps.  In particular we can decide that
__morestack doesn't clobber any unusual registers, if that is what is
causing the problem.

Ian

On 05/16/2017 12:19 PM, Ian Lance Taylor wrote:
> On Mon, May 15, 2017 at 10:00 PM, Daniel Santos <daniel.santos@pobox.com> wrote:
>> Ian, would you mind looking at this please?  A combination of my
>> -mcall-ms2sysv-xlogues patch with Bernd's patch is causing problems when
>> ix86_expand_split_stack_prologue() calls ix86_expand_call().
> I don't have a lot of context here.  I assume that ms2sysv is going to
> be used on Windows systems, where -fsplit-stack isn't really going to
> work anyhow, so I think it would probably be OK that reject that
> combination if it causes trouble.

Sorry I wasn't more specific.  This -mcall-ms2sysv-xlogues actually 
targets Wine, although they don't use -fsplit-stack.  My patch set as-is 
is disabled when fsplit-stack is used, but during 
ix86_compute_frame_layout, which is too late in the case of 
-fsplit-stack.  I think I should just change this to a sorry() in 
ix86_option_override_internal.

> Also, it's overkill for ix86_expand_split_stack_prologue to call
> ix86_expand_call.  The call is always to __morestack, and __morestack
> is written in assembler, so we could use a simpler version of
> ix86_expand_call if that helps.  In particular we can decide that
> __morestack doesn't clobber any unusual registers, if that is what is
> causing the problem.
>
> Ian

Well aside from the conflict of the two patches, it just looks like it 
has the potential to generate clobbers where none are needed, but I'm 
having trouble actually *proving* that, so maybe I'm just wrong.

Daniel