[PING] [PATCH] Fix ICE with x87 asm operands (PR inline-asm/68843)

[Bernd Edlinger]

Hi,

ping for the RTL optimization stuff.

The problem here is that the code in reg-stack.c
pretty much everywhere assumes that the stack registers
do not have gaps.  IMHO it is not worth to fix the
register allocation in a way that would be necessary for that
configuration to work correctly.

So this patch tries just to detect a situation that can't
possibly work and exit cleanly without raising any ICEs.

In this case we have a regstack of st(1) only. That is
temp_stack.top=0 and temp_stack.reg[0]=FIRST_STACK_REG+1.

So it is just by luck that the assertion in line 2522
triggers, because immediately before that we already
do std::swap (temp_stack[j], temp_stack[k]) with
j=-1 and k=0 in line 2118.  That is because of:

j = (temp_stack.top
     - (REGNO (recog_data.operand[i]) - FIRST_STACK_REG))

This formula only works, if you can assume that st(1) can
only be used in a stack that has at least two elements.

Likewise the return statement in get_hard_regnum:

return i >= 0 ? (FIRST_STACK_REG + regstack->top - i) : -1;

Which is also the same formula, and in this case
it returns FIRST_STACK_REG although the stack only has
one element FIRST_STACK_REG+1 aka st(1).



On 05/22/16 22:02, Uros Bizjak wrote:
> On Sun, May 22, 2016 at 9:00 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Hi!
>>
>> as described in the PR there are several non-intuitive rules that
>> one has to follow to avoid ICEs with x87 asm operands.
>>
>> This patch adds an explicit rule, that avoids ICE in the first test case and
>> removes an unnecessary error message in the second test case.
>>
>>
>> Boot-strapped and regression-tested on x86_64-pc-linux-gnu.
>> OK for trunk?
>
> This patch is actually dealing with two separate problems
>
> This part:
>
> @@ -607,7 +631,7 @@ check_asm_stack_operands (rtx_insn *insn)
>        record any earlyclobber.  */
>
>     for (i = n_outputs; i < n_outputs + n_inputs; i++)
> -    if (op_alt[i].matches == -1)
> +    if (op_alt[i].matches == -1 && STACK_REG_P (recog_data.operand[i]))
>         {
>    int j;
>
> is OK, although, I'd written it as:
>
>
> +    if (STACK_REG_P (recog_data.operand[i]) && op_alt[i].matches == -1)
>
> with slightly simplified testcase:
>
> --cut here--
> int
> __attribute__((noinline, noclone))
> test (double y)
> {
>    int a, b;
>    asm ("fistpl (%1)\n\t"
>         "movl (%1), %0"
>         : "=r" (a)
>         : "r" (&b), "t" (y)
>         : "st");
>    return a;
> }
>
> int
> main ()
> {
>    int t = -10;
>
>    if (test (t) != t)
>      __builtin_abort ();
>    return 0;
> }
> --cut here--
>
> BTW: It looks to me you also don't need all-memory clobber here.
>

right.

> This part is OK, with a testcase you provided it borders on obvious.
> However, you will need rtl-optimization approval for the other
> problem.
>
> Uros.
>

I changed the patch according to your comments, thanks.

I have the updated patch attached.


Thanks
Bernd.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: changelog-pr68843.txt
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20160529/7498088c/attachment.txt>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-pr68843.diff
Type: text/x-patch
Size: 3648 bytes
Desc: patch-pr68843.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20160529/7498088c/attachment.bin>