This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: RFC: LRA for x86/x86-64 [7/9] -- continuation

On 12-10-17 7:24 AM, Richard Sandiford wrote:
Thanks for all the updates.

Vladimir Makarov <vmakarov@redhat.com> writes: 
+	  /* index * scale + disp => new base + index * scale  */
+	  enum reg_class cl = base_reg_class (mode, as, SCRATCH, SCRATCH);
+
+	  lra_assert (INDEX_REG_CLASS != NO_REGS);
+	  new_reg = lra_create_new_reg (Pmode, NULL_RTX, cl, "disp");
+	  lra_assert (GET_CODE (*addr_loc) == PLUS);
+	  lra_emit_move (new_reg, *ad.disp_loc);
+	  if (CONSTANT_P (XEXP (*addr_loc, 1)))
+	    XEXP (*addr_loc, 1) = XEXP (*addr_loc, 0);
+	  XEXP (*addr_loc, 0) = new_reg;

The canonical form is (plus (mult ...) (reg)) rather than
(plus (reg) (mult ...)), but it looks like we create the latter.
I realise you try both forms here:
It might happen because equiv substitution in LRA. 
+	  /* Some targets like ARM, accept address operands in
+	     specific order -- try exchange them if necessary.	*/
+	  if (! valid_address_p (mode, *addr_loc, as))
+	    {
+	      exchange_plus_ops (*addr_loc);
+	      if (! valid_address_p (mode, *addr_loc, as))
+		exchange_plus_ops (*addr_loc);
+	    }

but I think we should try the canonical form first.  And I'd prefer it
if we didn't try the other form at all, especially in 4.8.  It isn't
really the backend's job to reject non-canonical rtl.  This might well
be another case where some targets need a (hopefully small) tweak in
order to play by the rules.

Also, I suppose this section of code feeds back to my question on
Wednesday about the distinction that LRA seems to make between the
compile-time constant in:

(plus (reg X1) (const_int Y1))

and the link-time constant in:

(plus (reg X2) (symbol_ref Y2))

It looked like extract_address_regs classified X1 as a base register and
X2 as an index register.  The difference between the two constants has
no run-time significance though, and I think we should handle both X1
and X2 as base registers (as I think reload does).

I think the path above would then be specific to scaled indices.
In the original address the "complex" index must come first and the
displacement second.  In the modified address, the index would stay
first and the new base register would be second.  More below.

As I wrote above the problem is also in that equiv substitution can
create non-canonical forms.

Right.  Just in case there's a misunderstanding: I'm not complaining
about these routines internally using forms that are noncanonical
(which could happen because of equiv substitution, like you say).
I just think that what we eventually try to validate should be canonical.
In a way it's similar to how the simplify-rtx.c routines work.

If there are targets that only accept noncanonical rtl (which is after
all just a specific type of invalid rtl), they need to be fixed.
Agree. In order not to forget to fix targets I am removing operand exchange. 
+      /* base + scale * index + disp => new base + scale * index  */
+      new_reg = base_plus_disp_to_reg (mode, as, &ad);
+      *addr_loc = gen_rtx_PLUS (Pmode, new_reg, *ad.index_loc);
+      if (! valid_address_p (mode, *addr_loc, as))
+	{
+	  /* Some targets like ARM, accept address operands in
+	     specific order -- try exchange them if necessary.	*/
+	  exchange_plus_ops (*addr_loc);
+	  if (! valid_address_p (mode, *addr_loc, as))
+	    exchange_plus_ops (*addr_loc);
+	}

Same comment as above about canonical rtl.  Here we can have two
registers -- in which case the base should come first -- or a more
complex index -- in which case the index should come first.

We should be able to pass both rtxes to simplify_gen_binary (PLUS, ...),
with the operands in either order, and let it take care of the details.
Using simplify_gen_binary would help with the earlier index+disp case too.

Equiv substitution can create non-canonical forms.  There are 2 approaches:
o have a code for dealing with non-canonical forms (equiv substitution,
target stupidity)
o always support canonical forms and require them from targets.

I decided to use the 1st variant but I am reconsidering it.  I'll try to
fix before inclusion.  But I am not sure I have time for this.  All
these changes makes LRA unstable. In fact, I've just found that changes
I already made so far resulted in 2 SPEC2000 tests broken although GCC
testsuite and bootstrap looks good.
OK. I'm happy to try fixing the noncanonical thing.

+  /* If this is post-increment, first copy the location to the reload reg.  */
+  if (post && real_in != result)
+    emit_insn (gen_move_insn (result, real_in));

Nit, but real_in != result can never be true AIUI, and I was confused how
the code could be correct in that case.  Maybe just remove it, or make
it an assert?
No, it might be true:

real_in = in == value ? incloc : in;
...
if (cond)
    result = incloc;
else
    result = ...

if (post && real_in != result)

So it is true if in==value && cond
Sorry, what I meant was that cond is "! post && REG_P (incloc)":

   if (! post && REG_P (incloc))
     result = incloc;
   else
     result = lra_create_new_reg (GET_MODE (value), value, new_rclass,
				 "INC/DEC result");

so it can never be true in the "post" case quoted above.


Fixed.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]