Reload/i386 patch for secondary memory vs subregs

Hello!

> Bootstrapped and tested on i686-linux. It's also been in several of our
> internal trees, going back to even 4.4-based ones IIRC, and has had
> testing for several architectures. Ok for the i386 part? I intend to
> check the reload bits in soon if there are no objections.


Is this correctness or performance issue?

SSE regs can hold SImode or DImode values, so it looks to me that this
limitation is true only for QI and HI modes.

Can you perhaps explain the reason for this limitation a bit more?

Uros.

On 08/03/2012 08:50 PM, Uros Bizjak wrote:
> Hello!
> 
>> Bootstrapped and tested on i686-linux. It's also been in several of our
>> internal trees, going back to even 4.4-based ones IIRC, and has had
>> testing for several architectures. Ok for the i386 part? I intend to
>> check the reload bits in soon if there are no objections.
> 
> Index: gcc/config/i386/i386.h
> ===================================================================
> --- gcc/config/i386/i386.h	(revision 189284)
> +++ gcc/config/i386/i386.h	(working copy)
> @@ -1376,7 +1376,8 @@ enum reg_class
>    ((MODE) == QImode && !TARGET_64BIT				\
>     && ((CLASS) == ALL_REGS || (CLASS) == GENERAL_REGS		\
>         || (CLASS) == LEGACY_REGS || (CLASS) == INDEX_REGS)	\
> -   ? Q_REGS : (CLASS))
> +   ? Q_REGS							\
> +   : (CLASS) == INT_SSE_REGS ? GENERAL_REGS : (CLASS))
> 
>  /* If we are copying between general and FP registers, we need a memory
>     location. The same is true for SSE and MMX registers.  */
> 
> Is this correctness or performance issue?
> 
> SSE regs can hold SImode or DImode values, so it looks to me that this
> limitation is true only for QI and HI modes.
> 
> Can you perhaps explain the reason for this limitation a bit more?

Without this, on the new testcase we hit the assert in
inline_secondary_memory_needed. The comment before the function states:

  The macro can't work reliably when one of the CLASSES is class
  containing registers from multiple units (SSE, MMX, integer).  We
  avoid this by never combining those units in single alternative in
  the machine description. Ensure that this constraint holds to avoid
  unexpected surprises.

So, this indicates that we shouldn't be using INT_SSE_REGS for a reload
class at all, and I expect that at the moment we don't. With the patch,
the new find_valid_class_1 discovers INT_SSE_REGS as the best class for
the register to hold the SYMBOL_REF, leading to the failed assert.


Bernd

On Fri, Aug 3, 2012 at 10:26 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 08/03/2012 08:50 PM, Uros Bizjak wrote:
>> Hello!
>>
>>> Bootstrapped and tested on i686-linux. It's also been in several of our
>>> internal trees, going back to even 4.4-based ones IIRC, and has had
>>> testing for several architectures. Ok for the i386 part? I intend to
>>> check the reload bits in soon if there are no objections.
>>
>> Index: gcc/config/i386/i386.h
>> ===================================================================
>> --- gcc/config/i386/i386.h    (revision 189284)
>> +++ gcc/config/i386/i386.h    (working copy)
>> @@ -1376,7 +1376,8 @@ enum reg_class
>>    ((MODE) == QImode && !TARGET_64BIT                         \
>>     && ((CLASS) == ALL_REGS || (CLASS) == GENERAL_REGS                \
>>         || (CLASS) == LEGACY_REGS || (CLASS) == INDEX_REGS)   \
>> -   ? Q_REGS : (CLASS))
>> +   ? Q_REGS                                                  \
>> +   : (CLASS) == INT_SSE_REGS ? GENERAL_REGS : (CLASS))
>>
>>  /* If we are copying between general and FP registers, we need a memory
>>     location. The same is true for SSE and MMX registers.  */
>>
>> Is this correctness or performance issue?
>>
>> SSE regs can hold SImode or DImode values, so it looks to me that this
>> limitation is true only for QI and HI modes.
>>
>> Can you perhaps explain the reason for this limitation a bit more?
>
> Without this, on the new testcase we hit the assert in
> inline_secondary_memory_needed. The comment before the function states:
>
>   The macro can't work reliably when one of the CLASSES is class
>   containing registers from multiple units (SSE, MMX, integer).  We
>   avoid this by never combining those units in single alternative in
>   the machine description. Ensure that this constraint holds to avoid
>   unexpected surprises.
>
> So, this indicates that we shouldn't be using INT_SSE_REGS for a reload
> class at all, and I expect that at the moment we don't. With the patch,
> the new find_valid_class_1 discovers INT_SSE_REGS as the best class for
> the register to hold the SYMBOL_REF, leading to the failed assert.

Thanks for the explanation!

This part is OK then, but please add the comment.

BTW: Perhaps we should use MAYBE_INTEGER_CLASS_P here?

Uros.