Re: [RFC][PATCH LRA] WIP patch to fix one part of PR87507

[Jeff Law <law at redhat dot com>]

On 10/27/18 10:24 AM, Peter Bergner wrote:
> On 10/22/18 6:45 PM, Peter Bergner wrote:
>> Bah, my bootstrap failed and I need to make a small change.  Let me do that
>> and verify my bootstraps get all the way thru before I give you the updated
>> patch.  Sorry.
> Ok, the following updated patch survives bootstrap and regtesting on
> powerpc64le-linux, x86_64-linux and s390x-linux with no regressions.
> Changes from the previous patch is that checking for illegal hard register
> usage in inline asm statements has been moved to expand time.  Secondly, the
> lra constraints code now checks for both HARD_REGISTER_P and REG_USERVAR_P.
> This was because I was seeing combine forcing hard regs into a pattern
> (not from inline asm) which lra needed to spill to match constraints,
> which it should be able to do.
> 
> Jeff, can you give this a try on your testers to see how it behaves on
> the other arches that were having problems?
> 
> Peter
> 
> 	PR rtl-optimization/87600
> 	* cfgexpand.c (expand_asm_stmt): Catch illegal asm constraint usage.
> 	* lra-constraints.c (process_alt_operands): Skip illegal hard
> 	register usage.  Prefer reloading non hard register operands.



> 

> Index: gcc/lra-constraints.c
> ===================================================================
> --- gcc/lra-constraints.c	(revision 265402)
> +++ gcc/lra-constraints.c	(working copy)
> @@ -2146,9 +2146,30 @@ process_alt_operands (int only_alternati
>  		      }
>  		    else
>  		      {
> -			/* Operands don't match.  Both operands must
> -			   allow a reload register, otherwise we
> -			   cannot make them match.  */
> +			/* Operands don't match.  If the operands are
> +			   different user defined explicit hard registers,
> +			   then we cannot make them match.  */
> +			if ((REG_P (*curr_id->operand_loc[nop])
> +			     || SUBREG_P (*curr_id->operand_loc[nop]))
> +			    && (REG_P (*curr_id->operand_loc[m])
> +				|| SUBREG_P (*curr_id->operand_loc[m])))
> +			  {
> +			    rtx nop_reg = *curr_id->operand_loc[nop];
> +			    if (SUBREG_P (nop_reg))
> +			      nop_reg = SUBREG_REG (nop_reg);
> +			    rtx m_reg = *curr_id->operand_loc[m];
> +			    if (SUBREG_P (m_reg))
> +			      m_reg = SUBREG_REG (m_reg);
So the one worry I have/had in this code is nested subregs.  My
recollection is they do happen in rare cases.  But I can also find a
reference  where Jim W. has indicated they're invalid (and I absolutely
trust Jim on this kind of historical RTL stuff).

https://gcc.gnu.org/ml/gcc/2005-04/msg01173.html

Reviewing cases where we've written the stripping as a loop, several are
stripping subregs as well as extractions.  And I can certainly believe
that we could have an RTX with some combination of subregs and
extractions.  The exception to that pattern would be reload which has
subreg stripping loops that only strip the subregs.

So, after all that, I think we're OK.  It might make sense to verify we
don't have nested subregs in the IL verifiers.  Bonus points if you add
that checking.

So ideally we'd have some tests which tickle this code, even if they're
target specific.

So I'm OK with this patch and also OK with adding tests independently as
a follow-up.

jeff