Subject: Re: [patch] MIPS/gcc: Revert removal of DImode shifts for 32-bit targets

From: Richard Henderson <[email-masked]>
Date: Thu, 2 Sep 2004 23:53:31 -0700

On Tue, Aug 31, 2004 at 08:51:20PM +0100, Richard Sandiford wrote:
> int TARGET_SHIFT_TRUNCATION_MASK (enum machine_mode MODE)
...
>      Note that, unlike `SHIFT_COUNT_TRUNCATED', this function does
>      _not_ apply to general shift rtxes; it applies only to instructions
>      that are generated by the named shift patterns.

I'm not particularly thrilled about this notion.  I'd much prefer a
target hook that could replace SHIFT_COUNT_TRUNCATED.  How often are
the named patterns going to differ from the rtxes anyway?

r~

---

From: Richard Sandiford <[email-masked]>
Date: Fri, 03 Sep 2004 08:05:15 +0100

> the named patterns going to differ from the rtxes anyway?

Well, the problem is that SHIFT_COUNT_TRUNCATED applies to all shift
rtxes, including those synthesised by things like combine.c:expand_
compound_operation().  I assume that's why SHIFT_COUNT_TRUNCATED is
documented as follows:

     A C expression that is nonzero if on this machine the number of bits
     actually used for the count of a shift operation is equal to the number
     of bits needed to represent the size of the object being shifted.  When
     this macro is nonzero, the compiler will assume that it is safe to omit
     a sign-extend, zero-extend, and certain bitwise `and' instructions that
     truncates the count of a shift operation.  On machines that have
     instructions that act on bit-fields at variable positions, which may
     include `bit test' instructions, a nonzero @code{SHIFT_COUNT_TRUNCATED}
     also enables deletion of truncations of the values that serve as
     arguments to bit-field instructions.

     If both types of instructions truncate the count (for shifts) and
     position (for bit-field operations), or if no variable-position bit-field
     instructions exist, you should define this macro.

     However, on some machines, such as the 80386 and the 680x0, truncation
     only applies to shift operations and not the (real or pretended)
     bit-field operations.  Define @code{SHIFT_COUNT_TRUNCATED} to be zero on
     such machines.  Instead, add patterns to the @file{md} file that include
     the implied truncation of the shift instructions.

I was deliberately trying to avoid this fuzziness with the new target hook.

E.g., if it ever becomes useful to know that ashlsi3 truncates on x86,
then it will be possible to use the new hook there too, even though the
requirements of S_C_T aren't met.

Richard

---

From: Richard Henderson <[email-masked]>
Date: Fri, 3 Sep 2004 00:08:58 -0700

> I was deliberately trying to avoid this fuzziness with the new target hook.

Hmm.  I suppose we could pass the shift operation in there; 
ASHIFT, LSHIFT, ZERO_EXTRACT, SIGN_EXTRACT.

r~

---

From: Richard Sandiford <[email-masked]>
Date: Fri, 03 Sep 2004 08:11:47 +0100

> ASHIFT, LSHIFT, ZERO_EXTRACT, SIGN_EXTRACT.

But the point as I understand it is that the generic optimisers
(e.g. simplify-rtx.c) can't tell the difference between an ASHIFT
that came from an (ashift ...) in the instruction stream or from
something that was generated artificially by expand_compound_operation.

Richard

---

From: Richard Henderson <[email-masked]>
Date: Fri, 3 Sep 2004 00:20:10 -0700

> something that was generated artificially by expand_compound_operation.

That would be a bug in expand_compound_operation, I would think.

The alternative is to not add your new hook and do what you can
with the existing SHIFT_COUNT_TRUNCATED macro.  Which I recommend
that you do; I don't think you really want to have the shift bits
dependent on a cleanup / infrastructure change of this scale.

r~

---

From: Richard Sandiford <[email-masked]>
Date: Fri, 03 Sep 2004 08:29:39 +0100

> dependent on a cleanup / infrastructure change of this scale.

FWIW, that's what my original patch did:

    http://gcc.gnu.org/ml/gcc-patches/2004-08/msg00461.html   

The patch I posted this week was in response to the request for
wider-ranging target support:

    http://gcc.gnu.org/ml/gcc-patches/2004-08/msg00606.html

But because it depended on S_C_T, the original patch produced much
worse code for ARM than the new one does.

Is the new target hook really that invasive?  It doesn't affect any
other code as such.

The only change not directly related to the optabs expansion was the
simplify-rtx.c thing, and like I said in my covering message, I think
that code's bogus anyway:

    This seems pretty dubious anyway.  What if a define_expand in the backend
    uses shifts to implement a complex named pattern?  I'd have thought the
    backend would be free to use target-specific knowledge about what that
    shift does with out-of-range values.  And if we are later able to
    constant-fold the result, the code above might not do what the
    target machine would do.

To be honest, I'd still like to apply that hunk even if we go back to S_C_T.

Richard

---

From: Richard Henderson <[email-masked]>
Date: Fri, 3 Sep 2004 13:15:10 -0700

>     http://gcc.gnu.org/ml/gcc-patches/2004-08/msg00606.html

Ah.  I wasn't actually asking for wider target support.  I was 
commenting that if we wanted something better, we'd have to add
new target support.  Sorry for the confusion.

> Is the new target hook really that invasive?  It doesn't affect any

No... I guess not.  And it is a start if we ever do decide to
expand its meaning to replace S_C_T.

> that code's bogus anyway ...

Agreed.

Ok, revised patch approved.

r~

---

From: Richard Sandiford <[email-masked]>
Date: Sat, 04 Sep 2004 09:53:28 +0100

Richard Henderson <[email-masked]> writes:
> new target support.

Ooops!  I misunderstood, sorry.

> expand its meaning to replace S_C_T.
[...]
> Ok, revised patch approved.

Thanks, applied.

Looking back, I see I didn't do a very good job of explaining why
I think S_C_T and this target hook are doing two different things.
A bit more explanation (mostly for the record, since I doubt I'm
saying anything surprising here):

I can only see two optimisations guarded by !S_C_T, both of them in
combine.c.  They only disallow S_C_T because we might have already
optimised the construct in a different way.  All other uses of S_C_T
are used for shifts.

So the important point is that: although S_C_T requires a particular
behaviour for things like ZERO_EXTRACT, it is never actually used in
a positive context for ZERO_EXTRACT rtxes.  S_C_T is only ever used
in a positive context for shift rtxes.

As I understand it, the reason that S_C_T has those extra requirements
is that we have no way of tracking where a particular shift came from.
Sometimes it might come from an insn's PATTERN, sometimes it might be
the result of some temporary rewrites, such as that performed by combine.c
for "compound" operations.  The definition of S_C_T says that these
rewrites must be valid.

It sounds like you're saying that the target hook should eventually
be extended to cover shift rtxes rather than shift optabs, and that
anything which generates a shift rtx should make sure the rtx behaves
correctly wrt these hooks.  So, for example, the onus for verifying the
ZERO_EXTRACT rewrites should be with combine rather than the backend.
Is that right?  If so, I'll try to look at that in the 3.6 timeframe.

Richard

---

From: Richard Henderson <[email-masked]>
Date: Sat, 4 Sep 2004 17:03:28 -0700

On Sat, Sep 04, 2004 at 09:53:28AM +0100, Richard Sandiford wrote:
> So, for example, the onus for verifying the ZERO_EXTRACT rewrites
> should be with combine rather than the backend.  Is that right?

Yes.

r~