[aarch64,RFA,rtl-optimization/87763] Fix insv_1 and insv_2 for aarch64

This is a generalized version of the patch I sent last week.  This
variant fixes the remainder of the insv_1 and insv_2 failures on
aarch64.   In combination with an updated patch from Steve this should
be enough to fix 87763.




A couple notes.

First, it re-uses combine's make_field_assignment in the aarch64
backend.  So we don't have to duplicate any of that logic.  I scanned
make_field_assignment and its children to make sure it didn't use any
data that was only valid during the combine pass, but I could have
missed something.  This is one of those cases where encapsulating the
pass specific bits in a class really helps avoid problems...  Just
saying....

Second, it relaxes the main insv pattern to allow an immediate in the
operand predicate, but forces it into a register during LRA.  I don't
generally like having predicates that are looser than the constraints,
but it really helps here.  Basically we want to see the constant field
we're going to insert.

Primarily looking for feedback from the aarch64 maintainers on the
pattern and Segher on the re-use of make_field_assignment.

I've bootstrapped and regression tested on aarch64 and aarch64_be as
well as x86_64, ppc64le, ppc64, i686, etc.  It's also been regression
tested on a nice variety of *-elf targets.

Segher, Richard/James OK for the trunk?

Jeff

Hi Jeff,

[...]

+  "#"
+  "&& 1"
+  [(const_int 0)]
+  "{
+     /* If we do not have an RMW operand, then copy the input
+ to the output before this insn.  Also modify the existing
+ insn in-place so we can have make_field_assignment actually
+ generate a suitable extraction.  */
+     if (!rtx_equal_p (operands[0], operands[1]))
+       {
+         emit_move_insn (operands[0], operands[1]);
+ XEXP (XEXP (SET_SRC (PATTERN (curr_insn)), 0), 0) = copy_rtx (operands[0]);
+       }
+
+     rtx make_field_assignment (rtx);
+     rtx newpat = make_field_assignment (PATTERN (curr_insn));
+     gcc_assert (newpat);
+     emit_insn (newpat);
+     DONE;

It seems that make_field_assignment returns a new pattern only  if it
succeeds and returns the same pattern otherwise. So I am wondering if
it is worth simplifying the above. Like removing the assert and
checking/inserting move only when new pattern is returned?

Thanks,
Kugan



>
> Jeff

On 4/22/19 6:39 PM, Kugan Vivekanandarajah wrote:
> Hi Jeff,
> 
> [...]
> 
> +  "#"
> +  "&& 1"
> +  [(const_int 0)]
> +  "{
> +     /* If we do not have an RMW operand, then copy the input
> + to the output before this insn.  Also modify the existing
> + insn in-place so we can have make_field_assignment actually
> + generate a suitable extraction.  */
> +     if (!rtx_equal_p (operands[0], operands[1]))
> +       {
> +         emit_move_insn (operands[0], operands[1]);
> + XEXP (XEXP (SET_SRC (PATTERN (curr_insn)), 0), 0) = copy_rtx (operands[0]);
> +       }
> +
> +     rtx make_field_assignment (rtx);
> +     rtx newpat = make_field_assignment (PATTERN (curr_insn));
> +     gcc_assert (newpat);
> +     emit_insn (newpat);
> +     DONE;
> 
> It seems that make_field_assignment returns a new pattern only  if it
> succeeds and returns the same pattern otherwise. So I am wondering if
> it is worth simplifying the above. Like removing the assert and
> checking/inserting move only when new pattern is returned?
Thanks, this is something I meant to clean up and just totally forgot.
If the assert ever triggers it means something has gone horribly wrong.
 Essentially the insn's condition wasn't sufficiently tight.

Catching it with the assert at this point is much easier to debug.  The
failure modes when you don't catch the failure to create a field
assignment here occur much later in the pipeline and it won't be obvious
why things went wrong (I know that from experience :-).

So please consider the assert to be

gcc_assert (newpat != PATTERN (curr_insn);

Jeff

Hi Jeff,

On Mon, Apr 22, 2019 at 09:09:12AM -0600, Jeff Law wrote:
> First, it re-uses combine's make_field_assignment in the aarch64
> backend.

That's not really acceptable.  make_field_assignment depends intimately
on make_extraction, which is very much combine-specific (and wrong in so
many ways it's hard to tell).

If you now use this in other places than combine, we will *never* be able
to fix it.  It's also shoddy architecture, of course.

> So we don't have to duplicate any of that logic.  I scanned
> make_field_assignment and its children to make sure it didn't use any
> data that was only valid during the combine pass, but I could have
> missed something.  This is one of those cases where encapsulating the
> pass specific bits in a class really helps avoid problems...  Just
> saying....

It isn't the data that is the problem.  combine has almost no private
data.  But the problem is all the hidden assumptions combine makes, and
for example that it often make **non-canonical** RTL; this is no problem
in combine, because that code will just not recog() (except of course
that you then get less well optimised code, in general).  (Often, combine
makes that RTL canonical later, fwiw).

But it _is_ a big problem in other contexts: you should not create
non-canonical RTL.  In your specific case, where you want to run this
from a splitter, you have to make sure you get only correct, recognised
patterns for your machine.  make_field_extraction cannot guarantee you
that.

> Second, it relaxes the main insv pattern to allow an immediate in the
> operand predicate, but forces it into a register during LRA.  I don't
> generally like having predicates that are looser than the constraints,
> but it really helps here.  Basically we want to see the constant field
> we're going to insert.

You also have an insn condition that can become invalid by the time the
pattern is split (the !reload_complted part), which means the pattern
will then *not* be split, which won't work here (the template is "#").

> -static int
> +int
>  get_pos_from_mask (unsigned HOST_WIDE_INT m, unsigned HOST_WIDE_INT *plen)

If you want to reuse this, please give it a better name, and move it to
rtlanal or some header file or similar?

It may be nicer to make a more machine-specific routine for this, see
rs6000_is_valid_mask for example.

> +;; This must be split before register allocation and reloading as
> +;; we need to verify it's actually an bitfield insertion by
> +;; examining both immediate operands.  After reload we will lose
> +;; knowledge of operands[3] constant status.

I don't understand what this means?  After reload operands[3] is still a
const_int?

> +(define_insn_and_split ""

Give this a name?  Starting with * if you don't want a gen_* for it.

> +  [(set (match_operand:GPI 0 "register_operand" "=r")
> +	(ior:GPI (and:GPI (match_operand:GPI 1 "register_operand" "0")
> +			  (match_operand:GPI 2 "const_int_operand" "n"))
> +		 (match_operand:GPI 3 "const_int_operand" "n")))]
> +  "(!reload_completed
> +    /* make_field_assignment doesn't handle subregs well.  */
> +    && REG_P (operands[0])

Another reason to not use make_field_assignment, I'm afraid :-(


Segher

On 4/24/19 7:05 AM, Segher Boessenkool wrote:
> Hi Jeff,
> 
> On Mon, Apr 22, 2019 at 09:09:12AM -0600, Jeff Law wrote:
>> First, it re-uses combine's make_field_assignment in the aarch64
>> backend.
> 
> That's not really acceptable.  make_field_assignment depends intimately
> on make_extraction, which is very much combine-specific (and wrong in so
> many ways it's hard to tell).
I think the structure of the pattern and its condition avoid the most
problematic issues.  But yea, it's not ideal.

>> So we don't have to duplicate any of that logic.  I scanned
>> make_field_assignment and its children to make sure it didn't use any
>> data that was only valid during the combine pass, but I could have
>> missed something.  This is one of those cases where encapsulating the
>> pass specific bits in a class really helps avoid problems...  Just
>> saying....
> 
> It isn't the data that is the problem.  combine has almost no private
> data.  But the problem is all the hidden assumptions combine makes, and
> for example that it often make **non-canonical** RTL; this is no problem
> in combine, because that code will just not recog() (except of course
> that you then get less well optimised code, in general).  (Often, combine
> makes that RTL canonical later, fwiw).
> 
> But it _is_ a big problem in other contexts: you should not create
> non-canonical RTL.  In your specific case, where you want to run this
> from a splitter, you have to make sure you get only correct, recognised
> patterns for your machine.  make_field_extraction cannot guarantee you
> that.
Even in the case where the form and operands are limited to what's in
the new pattern?  My worry with make_extraction is those cases where it
gets too smart for its own good and returns something even simpler than
extraction.  I'm not sure that can happen in this case or not.

I'm not keen on duplicating the logic from make_field_assignment and
make_extraction.  Though again, given the limited forms we accept it may
not be too bad.

> 
>> Second, it relaxes the main insv pattern to allow an immediate in the
>> operand predicate, but forces it into a register during LRA.  I don't
>> generally like having predicates that are looser than the constraints,
>> but it really helps here.  Basically we want to see the constant field
>> we're going to insert.
> 
> You also have an insn condition that can become invalid by the time the
> pattern is split (the !reload_complted part), which means the pattern
> will then *not* be split, which won't work here (the template is "#").
The pattern is split by the splitting pass before register allocation
and reload.  It's never supposed to exist beyond that splitting pass.
I'm pretty sure we've done this kind of thing regularly in the past.


> 
>> -static int
>> +int
>>  get_pos_from_mask (unsigned HOST_WIDE_INT m, unsigned HOST_WIDE_INT *plen)
> 
> If you want to reuse this, please give it a better name, and move it to
> rtlanal or some header file or similar?
If we go forward, yea, makes sense.

> 
> It may be nicer to make a more machine-specific routine for this, see
> rs6000_is_valid_mask for example.
> 
>> +;; This must be split before register allocation and reloading as
>> +;; we need to verify it's actually an bitfield insertion by
>> +;; examining both immediate operands.  After reload we will lose
>> +;; knowledge of operands[3] constant status.
> 
> I don't understand what this means?  After reload operands[3] is still a
> const_int?
It has to be an integer prior to register allocation.  Otherwise we
can't verify we have a valid field assignment.

The pattern should never exist past the insn splitting pass.  The
comment is obsolete.  It should always be split prior to register
allocation into a field assignment.



> 
>> +(define_insn_and_split ""
> 
> Give this a name?  Starting with * if you don't want a gen_* for it.
Didn't figure it was worth the trouble.  But it's easy enough to do.

> 
>> +  [(set (match_operand:GPI 0 "register_operand" "=r")
>> +	(ior:GPI (and:GPI (match_operand:GPI 1 "register_operand" "0")
>> +			  (match_operand:GPI 2 "const_int_operand" "n"))
>> +		 (match_operand:GPI 3 "const_int_operand" "n")))]
>> +  "(!reload_completed
>> +    /* make_field_assignment doesn't handle subregs well.  */
>> +    && REG_P (operands[0])
> 
> Another reason to not use make_field_assignment, I'm afraid :-(
Perhaps.  I'd probably want to avoid SUBREGs here regardless :-)


jeff

On Wed, Apr 24, 2019 at 12:16:39PM -0600, Jeff Law wrote:
> On 4/24/19 7:05 AM, Segher Boessenkool wrote:
> > On Mon, Apr 22, 2019 at 09:09:12AM -0600, Jeff Law wrote:
> >> First, it re-uses combine's make_field_assignment in the aarch64
> >> backend.
> > 
> > That's not really acceptable.  make_field_assignment depends intimately
> > on make_extraction, which is very much combine-specific (and wrong in so
> > many ways it's hard to tell).
> I think the structure of the pattern and its condition avoid the most
> problematic issues.  But yea, it's not ideal.

Oh, I'm not saying it does not work, I trust you tested it well.  But there
are so many ways this can completely break (with ICEs and everything), this
really isn't good.

You say all the problematic cases cannot happen...  So make a clone of this
function without any of those cases?  :-)

> > But it _is_ a big problem in other contexts: you should not create
> > non-canonical RTL.  In your specific case, where you want to run this
> > from a splitter, you have to make sure you get only correct, recognised
> > patterns for your machine.  make_field_extraction cannot guarantee you
> > that.
> Even in the case where the form and operands are limited to what's in
> the new pattern?  My worry with make_extraction is those cases where it
> gets too smart for its own good and returns something even simpler than
> extraction.  I'm not sure that can happen in this case or not.
> 
> I'm not keen on duplicating the logic from make_field_assignment and
> make_extraction.  Though again, given the limited forms we accept it may
> not be too bad.

Just praying that the RTL it creates is the RTL you prefer to see, or even
just RTL the backend can handle, doesn't sound too great.

(And any time this breaks it is a target bug, not combine).

> > You also have an insn condition that can become invalid by the time the
> > pattern is split (the !reload_complted part), which means the pattern
> > will then *not* be split, which won't work here (the template is "#").
> The pattern is split by the splitting pass before register allocation
> and reload.  It's never supposed to exist beyond that splitting pass.
> I'm pretty sure we've done this kind of thing regularly in the past.

Are you sure this code can never be created between the split pass and
when reload is finished?  Like maybe during LRA itself.  It's hard to
convince myself of this.

> >> +(define_insn_and_split ""
> > 
> > Give this a name?  Starting with * if you don't want a gen_* for it.
> Didn't figure it was worth the trouble.  But it's easy enough to do.

It helps debugging (and it helps naming stuff in the changelog :-) )

> >> +  [(set (match_operand:GPI 0 "register_operand" "=r")
> >> +	(ior:GPI (and:GPI (match_operand:GPI 1 "register_operand" "0")
> >> +			  (match_operand:GPI 2 "const_int_operand" "n"))
> >> +		 (match_operand:GPI 3 "const_int_operand" "n")))]
> >> +  "(!reload_completed
> >> +    /* make_field_assignment doesn't handle subregs well.  */
> >> +    && REG_P (operands[0])
> > 
> > Another reason to not use make_field_assignment, I'm afraid :-(
> Perhaps.  I'd probably want to avoid SUBREGs here regardless :-)

You get a subreg for using a 64-bit pseudo as 32-bit (a pseudo has only
one mode).  This happens a lot for insert insns, from what I see.


Segher