Re: [PATCH 2/2] combine: Don't turn (mult (extend x) 2^n) into extract

[Alex Coplan via Gcc-patches]

On 28/10/2020 09:09, Alex Coplan via Gcc-patches wrote:
> On 27/10/2020 17:31, Segher Boessenkool wrote:
> > On Tue, Oct 27, 2020 at 10:35:59AM +0000, Alex Coplan wrote:
> > > On 26/10/2020 12:43, Segher Boessenkool wrote:
> > > > I do not like handling both mult and ashift in one case like this, it
> > > > complicates things for no good reason.  Write it as two cases, and it
> > > > should be good.
> > > 
> > > OK, the attached patch rewrites (mult x 2^n) to (ashift x n) at the top
> > > of make_extraction so that the existing ASHIFT block can do the work for
> > > us. We remember if we did it and then convert it back if necessary.
> > > 
> > > I'm not convinced that it's an improvement. What do you think?
> > 
> > Restoring it like that is just yuck.  That can be okay if it is as the
> > start and end of a smallish function, importantly some self-contained
> > piece of code, but this is not.
> > 
> > Just write it as two blocks? One handling the shift, that is already
> > there; and add one block adding the mult case.  That should not
> > increase the complexity of this already way too complex code.
> 
> OK, how about the attached?
> 
> Bootstrap and regtest in progress on aarch64-none-linux-gnu.

This fails bootstrap since we trigger -Wsign-compare without the cast to
unsigned HOST_WIDE_INT on shift_amt:

> +      const HOST_WIDE_INT shift_amt = exact_log2 (INTVAL (XEXP (inner, 1)));
> +      if (shift_amt > 0 && len > shift_amt)

So, quoting an earlier reply:

> > +      const HOST_WIDE_INT shift_amt = (code == MULT) ? exact_log2 (ci) : 
> > ci;
> > +
> > +      if (shift_amt > 0 && len > (unsigned HOST_WIDE_INT)shift_amt)
> 
> Space after cast; better is to not need a cast at all (and you do not
> need one, len is unsigned HOST_WIDE_INT already).

unfortunately we do need the cast here.

See the revised patch attached, bootstrap on aarch64 in progress.

Thanks,
Alex

---

gcc/ChangeLog:

        * combine.c (make_extraction): Also handle shfits written as
        (mult x 2^n), avoid creating an extract rtx for these.

diff --git a/gcc/combine.c b/gcc/combine.c
index 4782e1d9dcc..729d04b1d9e 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -7665,6 +7665,24 @@ make_extraction (machine_mode mode, rtx inner, 
HOST_WIDE_INT pos,
       if (new_rtx != 0)
        return gen_rtx_ASHIFT (mode, new_rtx, XEXP (inner, 1));
     }
+  else if (GET_CODE (inner) == MULT
+          && CONST_INT_P (XEXP (inner, 1))
+          && pos_rtx == 0 && pos == 0)
+    {
+      /* We're extracting the least significant bits of an rtx
+        (mult X (const_int 2^C)), where LEN > C.  Extract the
+        least significant (LEN - C) bits of X, giving an rtx
+        whose mode is MODE, then multiply it by 2^C.  */
+      const HOST_WIDE_INT shift_amt = exact_log2 (INTVAL (XEXP (inner, 1)));
+      if (shift_amt > 0 && len > (unsigned HOST_WIDE_INT) shift_amt)
+       {
+         new_rtx = make_extraction (mode, XEXP (inner, 0),
+                                    0, 0, len - shift_amt,
+                                    unsignedp, in_dest, in_compare);
+         if (new_rtx)
+           return gen_rtx_MULT (mode, new_rtx, XEXP (inner, 1));
+       }
+    }
   else if (GET_CODE (inner) == TRUNCATE
           /* If trying or potentionally trying to extract
              bits outside of is_mode, don't look through