Question about make_extraction() in combine.c

[Michael Eager]

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]

Hi!

Post by Michael Eager
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.

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.

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.

I have a patch which removes this conversion. Works for my target. I
built and ran 'make check' for x86 with no additional failures. I don't
have a VAX or M68K sitting around to test. :-)

I can submit the patch and remove the restriction from the docs, but I
have no way of telling that this doesn't break (or deoptimize) some
other target.

[Jeff Law]

Post by Michael Eager

Hi!

Post by Michael Eager
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.

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.

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.
I have a patch which removes this conversion.  Works for my target.  I
built and ran 'make check' for x86 with no additional failures.  I don't
have a VAX or M68K sitting around to test.  :-)

I can do correctness tests for m68k via qemu. Essentially verifying it
still bootstraps:-) Just pass along a git formatted patch and I can
throw it into the tester.

jeff

[Segher Boessenkool]

Post by Michael Eager
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.

As another example, closer by, an extract length of 0 is not allowed
either, for zero_extract. And this *did* cause problems recently.

Why was it documented as requiring byte mode? Was this changed, just
the documentation was not updated?


Segher

[Michael Eager]

Post by Segher Boessenkool

Post by Michael Eager
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?

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

Post by Segher Boessenkool
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.)

Post by Segher Boessenkool
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.

[Segher Boessenkool]

Post by Michael Eager

Post by Segher Boessenkool

Post by Michael Eager
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.

Post by Michael Eager
The value of SET is VOIDmode, in that it has no value. Not sure what
your point is here.

See above.

Post by Michael Eager

Post by Segher Boessenkool
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.

Post by Michael Eager

Post by Segher Boessenkool
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

[Michael Eager]

Post by Segher Boessenkool

Post by Michael Eager

Post by Segher Boessenkool

Post by Michael Eager
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.

Essentially all other RTL operations place no restriction on mode.
If there are no arbitrary mode restrictions, then no checking is
required.

Post by Segher Boessenkool

Post by Michael Eager
The value of SET is VOIDmode, in that it has no value. Not sure what
your point is here.

See above.

Post by Michael Eager

Post by Segher Boessenkool
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.

These are not immutable rules delivered by some deity. Rules which
have no discernible value or purpose can and should be changed.

A patch is being tested.

Post by Segher Boessenkool

Post by Michael Eager

Post by Segher Boessenkool
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 :-/

Have at it.

[Jeff Law]

Post by Segher Boessenkool

Post by Michael Eager
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.

Post by Segher Boessenkool
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.

Post by Segher Boessenkool
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