RFA: cache enabled attribute by insn code

Christophe Lyon <christophe.lyon@linaro.org> writes:
> Hi,
>
> Commits 210964 and 210965 for this patch have broken GCC build on arm* targets.

Could you send me the .i?

> For instance on target arm-none-linux-gnueabi, when creating
> unwind-arm.o, I can see:
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:1362
> 0x8e3bcd process_insn_for_elimination
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1344
> 0x8e3bcd lra_eliminate(bool, bool)
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-eliminations.c:1408
> 0x8dd753 lra_constraints(bool)
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra-constraints.c:4049
> 0x8cdc1a lra(_IO_FILE*)
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/lra.c:2332
> 0x8911f8 do_reload
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5444
> 0x891508 execute
>         /tmp/5673443_3.tmpdir/aci-gcc-fsf/sources/gcc-fsf/trunk/gcc/ira.c:5605
> Please submit a full bug report,

Hmm, is this because of "insn_enabled"?  If so, how did that work before
the patch?  LRA already assumed that the "enabled" attribute didn't depend
on the operands.

Does the following patch help?

Thanks,
Richard


gcc/
	* config/arm/iterators.md (shiftable_ops): New code iterator.
	(t2_binop0): New code attribute.
	* config/arm/arm.md (insn_enabled): Delete.
	(enabled): Remove insn_enabled test.
	(*arith_shiftsi): Split out...
	(*arith_multsi): ...this pattern and remove insn_enabled attribute.

Richard Sandiford <rsandifo@linux.vnet.ibm.com> writes:
> Does the following patch help?

Bah, it won't of course: %i1 needs to be the operator.

Richard

On 27/05/14 15:08, Richard Sandiford wrote:
> Hmm, is this because of "insn_enabled"?  If so, how did that work before
> the patch?  LRA already assumed that the "enabled" attribute didn't depend
> on the operands.

Huh!  "enabled" can be applied to each alternative.  Alternatives are
selected based on the operands.  If LRA can't cope with that we have a
serious problem.  In fact, a pattern that matches but has no enabled
alternatives is meaningless and guaranteed to cause problems during
register allocation.

R.

On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
> On 27/05/14 15:08, Richard Sandiford wrote:
> > Hmm, is this because of "insn_enabled"?  If so, how did that work before
> > the patch?  LRA already assumed that the "enabled" attribute didn't depend
> > on the operands.
> 
> Huh!  "enabled" can be applied to each alternative.  Alternatives are
> selected based on the operands.  If LRA can't cope with that we have a
> serious problem.  In fact, a pattern that matches but has no enabled
> alternatives is meaningless and guaranteed to cause problems during
> register allocation.

This is not LRA fault, but the backend misusing the "enabled" attribute
for something it wasn't designed for, and IMHO against the documentation
of the attribute too.
Just look at the original submission why it has been added.

	Jakub

On 27/05/14 16:27, Jakub Jelinek wrote:
> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>> On 27/05/14 15:08, Richard Sandiford wrote:
>>> Hmm, is this because of "insn_enabled"?  If so, how did that work before
>>> the patch?  LRA already assumed that the "enabled" attribute didn't depend
>>> on the operands.
>>
>> Huh!  "enabled" can be applied to each alternative.  Alternatives are
>> selected based on the operands.  If LRA can't cope with that we have a
>> serious problem.  In fact, a pattern that matches but has no enabled
>> alternatives is meaningless and guaranteed to cause problems during
>> register allocation.
> 
> This is not LRA fault, but the backend misusing the "enabled" attribute
> for something it wasn't designed for, and IMHO against the documentation
> of the attribute too.
> Just look at the original submission why it has been added.
> 
> 	Jakub
> 

<quote>
The @code{enabled} insn attribute may be used to disable certain insn
alternatives for machine-specific reasons.
<quote>

The rest of the text just says what happens when this is done and then
gives an example usage.  It doesn't any time, either explicitly or
implicitly, say that this must be a static choice determined once-off at
run time.

That being said, I agree that this particular use case is pushing the
boundaries -- but that's always a risk when the boundaries aren't
clearly defined.

A better solution here would be to get rid of all those match_operator
patterns and replace them with iterators; but that's a lot of work,
particularly for all the conditinal operation patterns we have.  It
would probably also bloat the number of patterns quite alarmingly.

On Tue, May 27, 2014 at 04:40:13PM +0100, Richard Earnshaw wrote:
> <quote>
> The @code{enabled} insn attribute may be used to disable certain insn
> alternatives for machine-specific reasons.
> <quote>
> 
> The rest of the text just says what happens when this is done and then
> gives an example usage.  It doesn't any time, either explicitly or
> implicitly, say that this must be a static choice determined once-off at
> run time.

It says:
"This definition should be based on other insn attributes and/or target flags."
where from what I understand Andreas originally meant was that the other
insn attributes would be again based on target flags or other such insn
attributes.  Yeah, it explicitly rules out just basing the enabled attribute on
the operands, arm uses just another attribute based on the operands there,
but it still goes against the intent of the attribute, and is definitely a
nightmare for the register allocator.
Decisions based on the operands should be through constraints.

From what I can see, arm uses it just on a single insn, so I'd say it
shouldn't be a big deal to rewrite it.

	Jakub

On 27/05/14 16:50, Jakub Jelinek wrote:
> On Tue, May 27, 2014 at 04:40:13PM +0100, Richard Earnshaw wrote:
>> <quote>
>> The @code{enabled} insn attribute may be used to disable certain insn
>> alternatives for machine-specific reasons.
>> <quote>
>>
>> The rest of the text just says what happens when this is done and then
>> gives an example usage.  It doesn't any time, either explicitly or
>> implicitly, say that this must be a static choice determined once-off at
>> run time.
> 
> It says:
> "This definition should be based on other insn attributes and/or target flags."

<pedantry>
- It only says *should*, not *must*.  "Should" can imply a
recommendation, not a requirement.
- It only says *based on*, not *solely based on*.  Based on implies that
other information might be involved as well, without limitation.
</pedantary>

> where from what I understand Andreas originally meant was that the other
> insn attributes would be again based on target flags or other such insn
> attributes.  Yeah, it explicitly rules out just basing the enabled attribute on
> the operands, arm uses just another attribute based on the operands there,
> but it still goes against the intent of the attribute, and is definitely a
> nightmare for the register allocator.
> Decisions based on the operands should be through constraints.
> 
> From what I can see, arm uses it just on a single insn, so I'd say it
> shouldn't be a big deal to rewrite it.
> 

Pedantry aside, I'm not against making such a restriction; but it is a
new restriction that isn't explictly stated in the documentation today.

R.