Question about make_extraction() in combine.c

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Question about make_extraction() in combine.c

Michael Eager-2
In combine, simplify_comparison() is being called with the following
arguments:

   code = EQ
   op0 =  (and:SI (mem:SI (reg/v/f:SI 50 [ gp ]) (const_int 4 [0x4]))
   op1 =  (const_int 0 [0])
€
After churning down through make_compound_operation() and
make_compound_operation_int(), processing gets to make_extraction().
Eventually (combine.c:7753), make_extract() decides that the best
pattern is EP_epextzv.  No instruction patterns are provided for
"extz*"; get_best_reg_extraction_insn() returns false, and
make_extraction falls through to this code [indenting modified for
email].

combine.c:7786:
       /* Be careful not to go beyond the extracted object and
          maintain the natural alignment of the memory.  */
       wanted_inner_mode = smallest_int_mode_for_size (len);
       while (pos % GET_MODE_BITSIZE (wanted_inner_mode) + len
             > GET_MODE_BITSIZE (wanted_inner_mode))
        wanted_inner_mode = GET_MODE_WIDER_MODE
               (wanted_inner_mode).require ();

Len == 1; the result is that wanted_inner_mode == E_QImode.

No instruction patterns are provided for "extz*";
get_best_reg_extraction_insn() returns false, and make_extraction falls
through to this code

The (mem:SI) is converted to (mem:QI).

The return from make_extract() is
    (zero_extract:SI (mem:QI (reg/v/f:SI 50 [ gp ]))
       (const_int 1 [0x1])
       (const_int 2 [0x2]))

The target has an instruction pattern for zero_extract, but it matches
SI values, not QI.  So the instruction which implements a test of a bit
flag is never generated.

In an old version of GCC, a call to mode_dependent_address_p()
controlled whether this conversion from SI to QI was done.  This would
not be useful, since QI is a valid address mode in general, just not for
the zero_extract pattern.  A kludge was added to prevent this conversion
from SI to QI.  I'd like to avoid re-implementing the kludge.



Questions:
   1.  What is make_extract() trying to do with the MEM address?
   2.  Why modify the MEM address from SI to QI?
       There's no obvious benefit that I see of
       (zero_extract:SI (mem:QI)...)  over (zero_extract:SI (mem:SI)...).
   3.  What's the best way to fix this?
         - Remove the down-sizing of MEM in make_extract()?
         - Define patterns for extz*?
         - Do something so zero_extend accepts QI?

--
Michael Eager    [hidden email]
1960 Park Blvd., Palo Alto, CA 94306
Reply | Threaded
Open this post in threaded view
|

Re: Question about make_extraction() in combine.c

Segher Boessenkool
Hi!

On Wed, Nov 14, 2018 at 11:22:58AM -0800, Michael Eager wrote:

> The (mem:SI) is converted to (mem:QI).
>
> The return from make_extract() is
>    (zero_extract:SI (mem:QI (reg/v/f:SI 50 [ gp ]))
>       (const_int 1 [0x1])
>       (const_int 2 [0x2]))
>
> The target has an instruction pattern for zero_extract, but it matches
> SI values, not QI.  So the instruction which implements a test of a bit
> flag is never generated.

The RTL documentation says:

  If @var{loc} is in memory, its mode must be a single-byte integer mode.

But obviously various ports use other modes, too.  I wonder if that ever
worked well.


Segher
Reply | Threaded
Open this post in threaded view
|

Re: Question about make_extraction() in combine.c

Michael Eager-2
On 11/16/18 14:50, Segher Boessenkool wrote:

> Hi!
>
> On Wed, Nov 14, 2018 at 11:22:58AM -0800, Michael Eager wrote:
>> The (mem:SI) is converted to (mem:QI).
>>
>> The return from make_extract() is
>>     (zero_extract:SI (mem:QI (reg/v/f:SI 50 [ gp ]))
>>        (const_int 1 [0x1])
>>        (const_int 2 [0x2]))
>>
>> The target has an instruction pattern for zero_extract, but it matches
>> SI values, not QI.  So the instruction which implements a test of a bit
>> flag is never generated.
>
> The RTL documentation says:
>
>    If @var{loc} is in memory, its mode must be a single-byte integer mode.
>
> But obviously various ports use other modes, too.  I wonder if that ever
> worked well.

Thanks.  I hadn't noticed this.

Does anyone have any idea why there is a restriction on the mode?
Other instruction patterns don't place arbitrary restriction on the
memory access mode.



--
Michael Eager    [hidden email]
1960 Park Blvd., Palo Alto, CA 94306
Reply | Threaded
Open this post in threaded view
|

Re: Question about make_extraction() in combine.c

Jeff Law
On 11/18/18 8:44 AM, Michael Eager wrote:

> On 11/16/18 14:50, Segher Boessenkool wrote:
>> Hi!
>>
>> On Wed, Nov 14, 2018 at 11:22:58AM -0800, Michael Eager wrote:
>>> The (mem:SI) is converted to (mem:QI).
>>>
>>> The return from make_extract() is
>>>     (zero_extract:SI (mem:QI (reg/v/f:SI 50 [ gp ]))
>>>        (const_int 1 [0x1])
>>>        (const_int 2 [0x2]))
>>>
>>> The target has an instruction pattern for zero_extract, but it matches
>>> SI values, not QI.  So the instruction which implements a test of a bit
>>> flag is never generated.
>>
>> The RTL documentation says:
>>
>>    If @var{loc} is in memory, its mode must be a single-byte integer
>> mode.
>>
>> But obviously various ports use other modes, too.  I wonder if that ever
>> worked well.
>
> Thanks.  I hadn't noticed this.
>
> Does anyone have any idea why there is a restriction on the mode?
> Other instruction patterns don't place arbitrary restriction on the
> memory access mode.
Not offhand.   BUt it's been around for a long time (at least since the
early 90s).  I stumbled over it several times through the years.  It
could well be an artifact of the m68k or the vax.

Jeff
Reply | Threaded
Open this post in threaded view
|

Re: Question about make_extraction() in combine.c

Segher Boessenkool
On Wed, Nov 21, 2018 at 08:52:21AM -0800, Michael Eager wrote:

> On 11/21/2018 08:33 AM, Segher Boessenkool wrote:
> >On Tue, Nov 20, 2018 at 10:07:35AM -0800, Michael Eager wrote:
> >>The internal RTL should not be dictating what the target arch can or
> >>cannot implement.  Reload should insert any needed conversions,
> >>especially ones which narrow the size.
> >
> >Well, that depends.  A zero_extract of mem is only defined for byte_mode,
> >just like SET is only defined for VOIDmode (on the SET itself, not its
> >args).  Because this is guaranteed, nothing in GCC ever needs to check
> >this.  That is the theory of course; in reality quite a few targets have
> >used other modes for the mem in a zero_extract, and this seems to have
> >mostly worked.
>
> This restriction on zero_extract MEM args (and only MEM) seems to be
> completely arbitrary.  What is it about the operation of extracting a
> bit field which makes it dependent on the memory access size?

If the mode is required to be byte_mode, then all code dealing with it
can assume it to be byte_mode.  Without first having to check it, and
without having to handle other modes.

> The value of SET is VOIDmode, in that it has no value.   Not sure what
> your point is here.

See above.

> >As another example, closer by, an extract length of 0 is not allowed
> >either, for zero_extract.  And this *did* cause problems recently.
>
> This is a restriction which does make sense.  It isn't clear what
> the value of a zero length field is, or how to represent it.  If
> something is undefined, then there is a strong argument for making is
> invalid.  (Are there architectures which have instructions which extract
> a zero length bit field?  I doubt it.)

It doesn't matter if you (or I, or anyone) think it makes sense; the rules
are the rules.  If you want to change the rules, then post a patch.

> >Why was it documented as requiring byte mode?  Was this changed, just
> >the documentation was not updated?
>
> Ancient history.  As Jeff said, perhaps an architectural requirement of
> VAX or m68k.  This wasn't changed, as far as I'm aware.

So a mem in a *_extract is still required to be byte_mode you say?

I don't think we can change this in the documentation without reviewing
all code that deals with *_extract to see if this will work :-/


Segher
Reply | Threaded
Open this post in threaded view
|

Re: Question about make_extraction() in combine.c

Jeff Law
In reply to this post by Jeff Law
On 11/21/18 9:33 AM, Segher Boessenkool wrote:

> On Tue, Nov 20, 2018 at 10:07:35AM -0800, Michael Eager wrote:
>> The internal RTL should not be dictating what the target arch can or
>> cannot implement.  Reload should insert any needed conversions,
>> especially ones which narrow the size.
>
> Well, that depends.  A zero_extract of mem is only defined for byte_mode,
> just like SET is only defined for VOIDmode (on the SET itself, not its
> args).  Because this is guaranteed, nothing in GCC ever needs to check
> this.  That is the theory of course; in reality quite a few targets have
> used other modes for the mem in a zero_extract, and this seems to have
> mostly worked.
Right.

>
> As another example, closer by, an extract length of 0 is not allowed
> either, for zero_extract.  And this *did* cause problems recently.
Very true.  I don't doubt there's still corner cases that haven't been
well documented.

>
> Why was it documented as requiring byte mode?  Was this changed, just
> the documentation was not updated?
I don't think it was ever documented why byte mode was required, merely
that it was required.  I can recall stumbling over it in the early/mid
90s.  I've got references to the byte requirement going back to early
1992 in my archives.

After some wandering of those archives, folks were clearly worried about
the extv/insv insns reading/writing beyond the boundary of the current
object.  If you restrict memory ops to QImode, then you resolve that
problem.

But we're unlikely to get a definitive answer here.


I think the real question is can we reasonably lift the restriction.



Jeff