PR inline-asm/55934

Hello Vlad,

Attached is my attempt to fix PR55934, an error recovery issue in LRA
with incorrect constraints in an asm.

I'm not 100% sure this is all correct (especially the LRA insn data
invalidating in lra-assigns.c) but it appears to fix the PR without
introducing test suite failures.

Can you please review the patch?

Thanks,

Ciao!
Steven


gcc/
        PR inline-asm/55934
        * lra-assigns.c (assign_by_spills): Throw away the pattern of asms
        that have operands with impossible constraints.
        Add a FIXME for a speed-up opportunity.
        * lra-constraints.c (process_alt_operands): Verify that a class
        selected from constraints on asms is valid for the operand mode.
        (curr_insn_transform): Remove incorrect comment.

testsuite/
        PR inline-asm/55934
        * gcc.target/i386/pr55934.c: New test.

On Fri, Jan 18, 2013 at 12:45:06AM +0100, Steven Bosscher wrote:
> --- testsuite/gcc.target/i386/pr55934.c (revision 0)
> +++ testsuite/gcc.target/i386/pr55934.c (revision 0)
> @@ -0,0 +1,10 @@
> +/* PR inline-asm/55934 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target sse } */

Don't you need /* { dg-options "-msse" } */ ?  dg-require-effective-target sse
itself doesn't add -msse option...

> +_Complex float
> +foo (void)
> +{
> +  _Complex float x;
> +  __asm ("" : "=x" (x)); /* { dg-error "inconsistent register constraints" } */
> +  return x;
> +}

	Jakub

On Fri, Jan 18, 2013 at 12:59 AM, Jakub Jelinek wrote:
> On Fri, Jan 18, 2013 at 12:45:06AM +0100, Steven Bosscher wrote:
>> --- testsuite/gcc.target/i386/pr55934.c (revision 0)
>> +++ testsuite/gcc.target/i386/pr55934.c (revision 0)
>> @@ -0,0 +1,10 @@
>> +/* PR inline-asm/55934 */
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target sse } */
>
> Don't you need /* { dg-options "-msse" } */ ?  dg-require-effective-target sse
> itself doesn't add -msse option...

Actually, I needed even more than that:

/* { dg-options "-std=c99 -msse" } */

But I added that line after creating the diff, and forgot to update
the test case in the patch :-)

Ciao!
Steven

On 13-01-17 6:45 PM, Steven Bosscher wrote:
> Hello Vlad,
>
> Attached is my attempt to fix PR55934, an error recovery issue in LRA
> with incorrect constraints in an asm.
>
> I'm not 100% sure this is all correct (especially the LRA insn data
> invalidating in lra-assigns.c) but it appears to fix the PR without
> introducing test suite failures.
The code is correct but call lra_invalidate_insn_data is not necessary 
as the same thing will be done in lra_update_insn_recog_data (if what 
lra_invalidate_insn_data does is not done yet) .  So adding the 
additional call is harmless as the result will be the same.
> Can you please review the patch?
The patch is ok for me except I'd prefer not to call 
lra_invalidate_insn_data right before lra_update_insn_recog_data.

Thanks for working on the PR, Steven.
>
> gcc/
>          PR inline-asm/55934
>          * lra-assigns.c (assign_by_spills): Throw away the pattern of asms
>          that have operands with impossible constraints.
>          Add a FIXME for a speed-up opportunity.
>          * lra-constraints.c (process_alt_operands): Verify that a class
>          selected from constraints on asms is valid for the operand mode.
>          (curr_insn_transform): Remove incorrect comment.
>
> testsuite/
>          PR inline-asm/55934
>          * gcc.target/i386/pr55934.c: New test.
>
> Index: lra-assigns.c
> ===================================================================
> --- lra-assigns.c       (revision 195104)
> +++ lra-assigns.c       (working copy)
> @@ -1240,6 +1240,24 @@ assign_by_spills (void)
>                    asm_p = true;
>                    error_for_asm (insn,
>                                   "%<asm%> operand has impossible constraints");
> +                 /* Avoid further trouble with this insn.
> +                    For asm goto, instead of fixing up all the edges
> +                    just clear the template and clear input operands
> +                    (asm goto doesn't have any output operands).  */
> +                 if (JUMP_P (insn))
> +                   {
> +                     rtx asm_op = extract_asm_operands (PATTERN (insn));
> +                     ASM_OPERANDS_TEMPLATE (asm_op) = ggc_strdup ("");
> +                     ASM_OPERANDS_INPUT_VEC (asm_op) = rtvec_alloc (0);
> +                     ASM_OPERANDS_INPUT_CONSTRAINT_VEC (asm_op) =
> rtvec_alloc (0);
> +                     lra_invalidate_insn_data (insn);
> +                     lra_update_insn_recog_data (insn);
> +                   }
> +                 else
> +                   {
> +                     PATTERN (insn) = gen_rtx_USE (VOIDmode, const0_rtx);
> +                     lra_set_insn_deleted (insn);
> +                   }
>                  }
>              }
>            lra_assert (asm_p);
> @@ -1263,6 +1281,9 @@ assign_by_spills (void)
>            bitmap_ior_into (&changed_insns,
>                             &lra_reg_info[sorted_pseudos[i]].insn_bitmap);
>          }
> +
> +      /* FIXME: Look up the changed insns in the cached LRA insn data using
> +        an EXECUTE_IF_SET_IN_BITMAP over changed_insns.  */
>         FOR_EACH_BB (bb)
>          FOR_BB_INSNS (bb, insn)
>          if (bitmap_bit_p (&changed_insns, INSN_UID (insn)))
> Index: lra-constraints.c
> ===================================================================
> --- lra-constraints.c   (revision 195104)
> +++ lra-constraints.c   (working copy)
> @@ -1847,11 +1847,27 @@ process_alt_operands (int only_alternati
>                int const_to_mem = 0;
>                bool no_regs_p;
>
> +             /* If this alternative asks for a specific reg class, see if there
> +                is at least one allocatable register in that class.  */
>                no_regs_p
>                  = (this_alternative == NO_REGS
>                     || (hard_reg_set_subset_p
>                         (reg_class_contents[this_alternative],
>                          lra_no_alloc_regs)));
> +
> +             /* For asms, verify that the class for this alternative
> is possible
> +                for the mode that is specified.  */
> +             if (!no_regs_p && REG_P (op) && INSN_CODE (curr_insn) < 0)
> +               {
> +                 int i;
> +                 for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> +                   if (HARD_REGNO_MODE_OK (i, mode)
> +                       && in_hard_reg_set_p
> (reg_class_contents[this_alternative], mode, i))
> +                     break;
> +                 if (i == FIRST_PSEUDO_REGISTER)
> +                   winreg = false;
> +               }
> +
>                /* If this operand accepts a register, and if the
>                   register class has at least one allocatable register,
>                   then this operand can be reloaded.  */
> @@ -2742,10 +2758,6 @@ curr_insn_transform (void)
>          swap_operands (commutative);
>       }
>
> -  /* The operands don't meet the constraints.  goal_alt describes the
> -     alternative that we could reach by reloading the fewest operands.
> -     Reload so as to fit it.  */
> -
>     if (! alt_p && ! sec_mem_p)
>       {
>         /* No alternative works with reloads??  */
> Index: testsuite/gcc.target/i386/pr55934.c
> ===================================================================
> --- testsuite/gcc.target/i386/pr55934.c (revision 0)
> +++ testsuite/gcc.target/i386/pr55934.c (revision 0)
> @@ -0,0 +1,10 @@
> +/* PR inline-asm/55934 */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target sse } */
> +_Complex float
> +foo (void)
> +{
> +  _Complex float x;
> +  __asm ("" : "=x" (x)); /* { dg-error "inconsistent register constraints" } */
> +  return x;
> +}