[PATCH] combine: Fix simplify_comparison AND handling for WORD_REGISTER_OPERATIONS targets [PR109040]

[Jakub Jelinek <jakub@redhat.com>]

On Thu, Apr 06, 2023 at 12:15:51PM +0200, Eric Botcazou wrote:
> > Originally I didn't really see this as an operation.  But the more and
> > more I ponder it feels like it's an operation and thus should be subject
> > to WORD_REGISTER_OPERATIONS.
> > 
> > While it's not really binding on RTL semantics, if we look at how some
> > architectures implement reg->reg copies, they're actually implemented
> > with an ADD or IOR -- so a real operation under the hood.
> > 
> > If we accept a subreg copy as an operation and thus subject to
> > WORD_REGISTER_OPERATIONS then that would seem to imply the combine is
> > the real problem here.  Otherwise dse is the culprit.
> 
> Yes, I agree that there is an ambiguity for subreg copy operations.  At some 
> point I tried to define what register operations are and added a predicate to 
> that effect (word_register_operation_p ); while it returns true for SUBREG, 
> it's an opt-out predicate so this does not mean much.
> 
> I don't think that DSE does anything wrong: as I wrote in the PR, defining 
> WORD_REGISTER_OPERATIONS should not prevent any particular form of RTL.
> 
> I therefore think that the problem is in the combiner and probably in the 
> intermediate step shown by Jakub:
> 
> "Then after that try_combine we do:
> 13325           record_value_for_reg (dest, record_dead_insn,
> 13326                                 WORD_REGISTER_OPERATIONS
> 13327                                 && word_register_operation_p (SET_SRC 
> (setter))
> 13328                                 && paradoxical_subreg_p (SET_DEST 
> (setter))
> 13329                                 ? SET_SRC (setter)
> 13330                                 : gen_lowpart (GET_MODE (dest),
> 13331                                                SET_SRC (setter)));
> and the 3 conditions are true here and so record value of the whole setter.
> That then records among other things nonzero_bits as 0x8084c."
> 
> That's a recent addition of mine (ae20d760b1ed69f631c3bf9351bf7e5005d52297) 
> and I think that it probably abuses WORD_REGISTER_OPERATIONS and should either 
> be reverted or restricted to the load case documented in its comment.  I can 
> provide testing on SPARC if need be.

If we want to fix it in the combiner, I think the fix would be following.
The optimization is about
(and:SI (subreg:SI (reg:HI xxx) 0) (const_int 0x84c))
and IMHO we can only optimize it into
(subreg:SI (and:HI (reg:HI xxx) (const_int 0x84c)) 0)
if we know that the upper bits of the REG are zeros.  The optimization
is only when the constant second AND operand is the same in both modes.
Otherwise because of nonzero_bits ((reg:SI xxx), SImode) == 0x8084c
we optimize into (subreg:SI (reg:HI xxx) 0).
The original form has the upper 16 bits guaranteed to be all zeros,
while the new form has them whatever value comes from the original
operation.  For !WORD_REGISTER_OPERATIONS, we don't optimize this way
paradoxical SUBREGs at all, then by definition the upper bits are unspecified.

Now, this patch fixes the PR, but certainly generates worse (but correct)
code than the dse.cc patch.  So perhaps we want both of them?

As before, I unfortunately can't test it on riscv-linux (could perhaps try
that on sparc-solaris on GCC Farm which is another WORD_REGISTER_OPERATIONS
target, but last my bootstrap attempt there failed miserably because of the
Don't bootstrap at midnight issue in cp/Make-lang.in; I'll post a patch
for that once I test it).

2023-04-06  Jakub Jelinek  <jakub@redhat.com>

	PR target/109040
	* combine.cc (simplify_comparison) <case AND>: For swapping AND and
	SUBREG with paradoxical subreg on WORD_REGISTER_OPERATIONS target,
	ensure the upper bits are known to be 0.

--- gcc/combine.cc.jj	2023-02-28 11:28:56.423182357 +0100
+++ gcc/combine.cc	2023-04-06 12:02:31.694747361 +0200
@@ -12671,7 +12671,11 @@ simplify_comparison (enum rtx_code code,
 		  && ((WORD_REGISTER_OPERATIONS
 		       && mode_width > GET_MODE_PRECISION (tmode)
 		       && mode_width <= BITS_PER_WORD
-		       && trunc_int_for_mode (c1, tmode) == (HOST_WIDE_INT) c1)
+		       && trunc_int_for_mode (c1, tmode) == (HOST_WIDE_INT) c1
+		       /* Make sure the bits above tmode in the SUBREG_REG
+			  are all known to be 0, see PR109040.  */
+		       && (nonzero_bits (SUBREG_REG (XEXP (op0, 0)), mode)
+			   & ~GET_MODE_MASK (tmode)) == 0)
 		      || (mode_width <= GET_MODE_PRECISION (tmode)
 			  && subreg_lowpart_p (XEXP (op0, 0))))
 		  && mode_width <= HOST_BITS_PER_WIDE_INT


	Jakub