[01/10] Fix LRA bug

[This is new patch not included in the previously posted patch sets.]

This patch fixes an ICE building libgfortran/random.c.

The problem was an adddi3 instruction that had an eliminable frame pointer.
GCN adddi3 includes a match_scratch, which LRA substitutes with a REG, and
checks if it can be converted back to a scratch afterwards.  In the meantime,
the add was converted to a move, meaning that the instruction pattern
completely changed, thus causing a segfault when the instruction is revisited
in restore_scratches.

2018-12-12  Andrew Stubbs  <ams@codesourcery.com>

	gcc/
	* gcc/lra-int.h (lra_register_new_scratch_op): Add third parameter.
	* gcc/lra-remat.c (update_scratch_ops): Pass icode to
	lra_register_new_scratch_op.
	* gcc/lra.c (struct sloc): Add icode field.
	(lra_register_new_scratch_op): Add icode parameter.
	Use icode to skip insns that have changed beyond recognition.
---
 gcc/lra-int.h   |  2 +-
 gcc/lra-remat.c |  2 +-
 gcc/lra.c       | 12 ++++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

On 12/12/18 4:52 AM, Andrew Stubbs wrote:
> 
> [This is new patch not included in the previously posted patch sets.]
> 
> This patch fixes an ICE building libgfortran/random.c.
> 
> The problem was an adddi3 instruction that had an eliminable frame pointer.
> GCN adddi3 includes a match_scratch, which LRA substitutes with a REG, and
> checks if it can be converted back to a scratch afterwards.  In the meantime,
> the add was converted to a move, meaning that the instruction pattern
> completely changed, thus causing a segfault when the instruction is revisited
> in restore_scratches.
I'm torn here.  There's undocumented rules about this kind of stuff,
largely inherited from the days of "reload".  But I don't actually
recall the specifics of any of those rules.

I don't think there's any inherently wrong with what you've done in LRA.
 My worry is there's other places where this kind of changing the
structure of the underlying insn is going to cause you problems if your
patterns aren't following those undocumented rules.




> 
> 2018-12-12  Andrew Stubbs  <ams@codesourcery.com>
> 
> 	gcc/
> 	* gcc/lra-int.h (lra_register_new_scratch_op): Add third parameter.
> 	* gcc/lra-remat.c (update_scratch_ops): Pass icode to
> 	lra_register_new_scratch_op.
> 	* gcc/lra.c (struct sloc): Add icode field.
> 	(lra_register_new_scratch_op): Add icode parameter.
> 	Use icode to skip insns that have changed beyond recognition.
OK.  But be aware we may have to revisit and look more closely what what
you're doing in your port if we stumble over more problems with reload
changing the structure of your insns and causing problems in the process.

jeff

On 13/12/2018 23:49, Jeff Law wrote:
> OK.  But be aware we may have to revisit and look more closely what what
> you're doing in your port if we stumble over more problems with reload
> changing the structure of your insns and causing problems in the process.

Thanks.

What's novel about this, I think, is that we have two candidates for an 
add instruction, and each clobbers a different condition register. I had 
previously handled this by clobbering both, but that was unsatisfactory, 
so I've changed it to use the match_scratch trick I found used in other 
ports.

Using a match_scratch is clearly common practice, but there does appear 
to have been an assumption in LRA that this wouldn't occur within 
patterns that can be transformed by register elimination. Presumably 
those other ports using match_scratch are not using it for some key 
patterns, or do not use register elimination.

Anyway, this patch should not affect any use case that did not already 
have UB, so I'll get it committed shortly.

Andrew

On 14/12/2018 10:04, Andrew Stubbs wrote:
> Anyway, this patch should not affect any use case that did not already 
> have UB, so I'll get it committed shortly.

Now done. Thanks for the review.

Andrew