[LRA] Fix ICE on pathological testcase

[LRA] Fix ICE on pathological testcase

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

Hi,

the compiler ICEs for SPARC 64-bit with LRA on the asm-subreg-1.c test:

volatile unsigned short _const_32 [4] = {1,2,3,4};
void
evas_common_convert_yuv_420p_601_rgba()
{
  __asm__ __volatile__ ("" : : "X" (*_const_32));
}

The issue is that combine merges back the 3 instructions necessary to build 
the address of _const_32 into a big MEM expression:

(insn 10 9 0 2 (asm_operands/v ("") ("") 0 [
            (mem/v/c:HI (lo_sum:DI (mult:DI (lo_sum:DI (high:DI (unspec:DI [
                                        (symbol_ref:DI ("_const_32") [flags 
0x2]  <var_decl 0x7ffff7ff6630 _const_32>)
                                    ] UNSPEC_SETH44))
                            (unspec:DI [
                                    (symbol_ref:DI ("_const_32") [flags 0x2]  
<var_decl 0x7ffff7ff6630 _const_32>)
                                ] UNSPEC_SETM44))
                        (const_int 4096 [0x1000]))
                    (symbol_ref:DI ("_const_32") [flags 0x2]  <var_decl 
0x7ffff7ff6630 _const_32>)) [1 _const_32+0 S2 A16])
        ]
         [
            (asm_input:HI ("X") asm-subreg-1.c:13)
        ]
         [] asm-subreg-1.c:13) "asm-subreg-1.c":13 -1
     (nil))

and LRA calls decompose_mem_address on the address, which aborts out of 
confusion; reload (and all subsequent passes) lets it go through unmodified.

The attached patch simply adds a bypass to process_address_1 in order to avoid 
invoking decompose_mem_address in this case.

Tested on SPARC/Solaris with LRA and x86-64/Linux, OK for the mainline?


2016-12-06  Eric Botcazou  <ebotcazou@adacore.com>

	* lra-constraints.c (process_address_1): Do not attempt to decompose
	addresses for MEMs that satisfy fixed-form constraints.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 728 bytes --]

Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 243245)
+++ lra-constraints.c	(working copy)
@@ -3080,7 +3080,11 @@ process_address_1 (int nop, bool check_o
 
   if (insn_extra_address_constraint (cn))
     decompose_lea_address (&ad, curr_id->operand_loc[nop]);
-  else if (MEM_P (op))
+  /* Do not attempt to decompose arbitrary addresses generated by combine
+     for asm operands with loose constraints, e.g 'X'.  */
+  else if (MEM_P (op)
+	   && !(get_constraint_type (cn) == CT_FIXED_FORM
+	        && constraint_satisfied_p (op, cn)))
     decompose_mem_address (&ad, op);
   else if (GET_CODE (op) == SUBREG
 	   && MEM_P (SUBREG_REG (op)))

Re: [LRA] Fix ICE on pathological testcase

[Jeff Law]

On 12/06/2016 05:28 AM, Eric Botcazou wrote:
> Hi,
>
> the compiler ICEs for SPARC 64-bit with LRA on the asm-subreg-1.c test:
>
> volatile unsigned short _const_32 [4] = {1,2,3,4};
> void
> evas_common_convert_yuv_420p_601_rgba()
> {
>   __asm__ __volatile__ ("" : : "X" (*_const_32));
> }
>
> The issue is that combine merges back the 3 instructions necessary to build
> the address of _const_32 into a big MEM expression:
>
> (insn 10 9 0 2 (asm_operands/v ("") ("") 0 [
>             (mem/v/c:HI (lo_sum:DI (mult:DI (lo_sum:DI (high:DI (unspec:DI [
>                                         (symbol_ref:DI ("_const_32") [flags
> 0x2]  <var_decl 0x7ffff7ff6630 _const_32>)
>                                     ] UNSPEC_SETH44))
>                             (unspec:DI [
>                                     (symbol_ref:DI ("_const_32") [flags 0x2]
> <var_decl 0x7ffff7ff6630 _const_32>)
>                                 ] UNSPEC_SETM44))
>                         (const_int 4096 [0x1000]))
>                     (symbol_ref:DI ("_const_32") [flags 0x2]  <var_decl
> 0x7ffff7ff6630 _const_32>)) [1 _const_32+0 S2 A16])
>         ]
>          [
>             (asm_input:HI ("X") asm-subreg-1.c:13)
>         ]
>          [] asm-subreg-1.c:13) "asm-subreg-1.c":13 -1
>      (nil))
>
> and LRA calls decompose_mem_address on the address, which aborts out of
> confusion; reload (and all subsequent passes) lets it go through unmodified.
>
> The attached patch simply adds a bypass to process_address_1 in order to avoid
> invoking decompose_mem_address in this case.
>
> Tested on SPARC/Solaris with LRA and x86-64/Linux, OK for the mainline?
>
>
> 2016-12-06  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* lra-constraints.c (process_address_1): Do not attempt to decompose
> 	addresses for MEMs that satisfy fixed-form constraints.
Presumably the MEM isn't a valid memory address, but it's allowed 
through due to the use of an "X" constraint?

ISTM that LRA has to be prepared to handle an arbitrary RTX, so punting 
seems appropriate.

jeff

Re: [LRA] Fix ICE on pathological testcase

[Eric Botcazou]

> Presumably the MEM isn't a valid memory address, but it's allowed
> through due to the use of an "X" constraint?

Yes (that was supposed to be more or less clear given the description :-).

> ISTM that LRA has to be prepared to handle an arbitrary RTX, so punting
> seems appropriate.

My opinion too.  Note that Bernd E. has another solution though.

-- 
Eric Botcazou

Re: [LRA] Fix ICE on pathological testcase

[Jeff Law]

On 12/07/2016 04:21 PM, Eric Botcazou wrote:
>> Presumably the MEM isn't a valid memory address, but it's allowed
>> through due to the use of an "X" constraint?
>
> Yes (that was supposed to be more or less clear given the description :-).
>
>> ISTM that LRA has to be prepared to handle an arbitrary RTX, so punting
>> seems appropriate.
>
> My opinion too.  Note that Bernd E. has another solution though.
>
I wasn't ever able to get comfortable with the change in behavior that 
Bernd E's patch introduced.  Namely that X no longer meant "anything", 
which is how it's been documented for eons.

Jeff

Re: [LRA] Fix ICE on pathological testcase

[Eric Botcazou]

> I wasn't ever able to get comfortable with the change in behavior that
> Bernd E's patch introduced.  Namely that X no longer meant "anything",
> which is how it's been documented for eons.

Understood.  I have applied my patchlet then, with the other, more serious LRA 
patch I posted yesterday, it yields a clean C testsuite in 64-bit mode too.

-- 
Eric Botcazou

Re: [LRA] Fix ICE on pathological testcase

[Bernd Edlinger]

On 12/07/16 10:44, Richard Sandiford wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>> Hi Eric,
>>
>> what you got there, looks more or less exactly like what I tried
>> to fix with that patch a few months ago, but unfortunately
>> we were unable to get a consensus on that approach:
>>
>> [PATCH] Fix asm X constraint (PR inline-asm/59155)
>> https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01649.html
>
> Does your patch also fix Eric's testcase?  If so then that approach
> looks safer to me FWIW.  The problem with allowing arbitrary MEMs
> is that it's just not clear:
>

Yes.  I tried the latest patch from here:
https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01363.html

And built a sparc64-elf cross-compiler with that.  It fixes the
reported crash and makes the asm argument even printable.

> (a) which other parts of the compiler (including target code) needs
>     to handle these arbitrary MEMs.  One instance is the target-specific
>     code for printing an address, as Jakub says in that thread, but it
>     could also affect things like cost calculations, etc.
>
> (b) should a spilled pseudo in the MEM be replaced with a MEM itself,
>     so that even RISC targets have to support nested MEMs?  Or should
>     pseudo registers be reloaded?  If so, into which register classes?
>     Should they be divided into base and index registers in the normal
>     way?  That's difficult to do if we can't decompose the address.
>

(c) there is no way how to test these unreasonably complex RTXes.
     They are created just by random optimizations.

> Reload's always been a recurring source of problems, so the fact
> that this particular case went through without an ICE doesn't mean
> that arbitrary MEMs were actually safe in general.
>

There are not safe at all.  There are lots of different ways how we can
ICE with asm X constraints.


Jeff, what should we do here?


Thanks
Bernd.

Re: [LRA] Fix ICE on pathological testcase

[Richard Sandiford]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> Hi Eric,
>
> what you got there, looks more or less exactly like what I tried
> to fix with that patch a few months ago, but unfortunately
> we were unable to get a consensus on that approach:
>
> [PATCH] Fix asm X constraint (PR inline-asm/59155)
> https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01649.html

Does your patch also fix Eric's testcase?  If so then that approach
looks safer to me FWIW.  The problem with allowing arbitrary MEMs
is that it's just not clear:

(a) which other parts of the compiler (including target code) needs
    to handle these arbitrary MEMs.  One instance is the target-specific
    code for printing an address, as Jakub says in that thread, but it
    could also affect things like cost calculations, etc.

(b) should a spilled pseudo in the MEM be replaced with a MEM itself,
    so that even RISC targets have to support nested MEMs?  Or should
    pseudo registers be reloaded?  If so, into which register classes?
    Should they be divided into base and index registers in the normal
    way?  That's difficult to do if we can't decompose the address.

Reload's always been a recurring source of problems, so the fact
that this particular case went through without an ICE doesn't mean
that arbitrary MEMs were actually safe in general.

Thanks,
Richard

Re: [LRA] Fix ICE on pathological testcase

[Mike Stump]

On Dec 6, 2016, at 8:59 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
>> what you got there, looks more or less exactly like what I tried
>> to fix with that patch a few months ago, but unfortunately
>> we were unable to get a consensus on that approach:
> 
> It's indeed the same underlying issue, but restricted to a specific case where 
> reload and other passes do the right thing, so LRA needs to be on par IMO.

So, the only use of X I make during development (other than match_scratch), would be to debug and develop.  In that context, I don't want any ICEs, and the semantics I want are no processing of the operand, and allow me to print it out (but not for the assembler, just a comment to read and see what it was).  Doesn't strike me as unreasonable and I can't think of any better or sane semantics.  Would be nice to allow those semantics as an ICE isn't as useful.

So, from that perspective, I don't think patches to fix ICEs should be rejected on that basis alone.

Re: [LRA] Fix ICE on pathological testcase

[Eric Botcazou]

> what you got there, looks more or less exactly like what I tried
> to fix with that patch a few months ago, but unfortunately
> we were unable to get a consensus on that approach:

It's indeed the same underlying issue, but restricted to a specific case where 
reload and other passes do the right thing, so LRA needs to be on par IMO.

-- 
Eric Botcazou

Re: [LRA] Fix ICE on pathological testcase

[Bernd Edlinger]

Hi Eric,

what you got there, looks more or less exactly like what I tried
to fix with that patch a few months ago, but unfortunately
we were unable to get a consensus on that approach:

[PATCH] Fix asm X constraint (PR inline-asm/59155)
https://gcc.gnu.org/ml/gcc-patches/2016-06/msg01649.html


Bernd.