One more patch for PR89676

Message ID 02bf51a9-532d-8fdc-a2b2-083cee308eb7@redhat.com
State New
Headers show
Series
  • One more patch for PR89676
Related show

Commit Message

Vladimir Makarov March 25, 2019, 9:22 p.m.
Jeff Law recently found that my latest patch break some existing code 
compilation (the code is big to make test out of it).

Here is the patch to fix it.  The patch was successfully bootstrapped on 
x86-64.  The patch actually results in less new transformations the 
previous patch introduced.  So it should be safe.

Committed as rev. 269924.

Comments

Maxim Kuvyrkov March 26, 2019, 8:25 a.m. | #1
> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:

> 

>   Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it).

> 

> Here is the patch to fix it.  The patch was successfully bootstrapped on x86-64.  The patch actually results in less new transformations the previous patch introduced.  So it should be safe.

> 

> Committed as rev. 269924.


Hi Vladimir,

FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused.

The failure was:
===
slub.s: Assembler messages:
slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]'
===

from a reduced testcase:
===
void *a;
long b, c;
void d(void) {
  typeof(0) e=0;
  asm("	casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n"
      : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a)
      : [new1] "r"(d), [new2] "r"(e));
}
===

Is this the same bug that Jeff reported?

Thanks,

--
Maxim Kuvyrkov
www.linaro.org
Vladimir Makarov March 26, 2019, 12:20 p.m. | #2
On 3/26/19 4:25 AM, Maxim Kuvyrkov wrote:
>> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:

>>

>>    Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it).

>>

>> Here is the patch to fix it.  The patch was successfully bootstrapped on x86-64.  The patch actually results in less new transformations the previous patch introduced.  So it should be safe.

>>

>> Committed as rev. 269924.

> Hi Vladimir,

>

> FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused.

>

> The failure was:

> ===

> slub.s: Assembler messages:

> slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]'

> ===

>

> from a reduced testcase:

> ===

> void *a;

> long b, c;

> void d(void) {

>    typeof(0) e=0;

>    asm("	casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n"

>        : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a)

>        : [new1] "r"(d), [new2] "r"(e));

> }

> ===

>

> Is this the same bug that Jeff reported?

>

Yes, it looks similar (reg pair and casp).  I did know it was a kernel.  I only had a preprocessed file slub.i.

The reduced case is actually wrong.  The constraints +&r does not guarantee that b and c will be in the subsequent regs.  In the original test case b and c were also local *register* variables (x0 and x1).  Generally speaking even this does no guarantee (according to the documentation) that the original variable values will be in subsequent regs for casp.  But as people assume such (undocumented) semantics, we should maintain this.
Maxim Kuvyrkov March 26, 2019, 12:35 p.m. | #3
> On Mar 26, 2019, at 3:20 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:

> 

> On 3/26/19 4:25 AM, Maxim Kuvyrkov wrote:

>>> On Mar 26, 2019, at 12:22 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:

>>> 

>>>   Jeff Law recently found that my latest patch break some existing code compilation (the code is big to make test out of it).

>>> 

>>> Here is the patch to fix it.  The patch was successfully bootstrapped on x86-64.  The patch actually results in less new transformations the previous patch introduced.  So it should be safe.

>>> 

>>> Committed as rev. 269924.

>> Hi Vladimir,

>> 

>> FWIW, this fixed linux kernel builds on AArch64 and ARM, which your first patch caused.

>> 

>> The failure was:

>> ===

>> slub.s: Assembler messages:

>> slub.s:26: Error: reg pair must start from even reg at operand 1 -- `casp x1,x0,x3,x5,[x6]'

>> ===

>> 

>> from a reduced testcase:

>> ===

>> void *a;

>> long b, c;

>> void d(void) {

>>   typeof(0) e=0;

>>   asm("	casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n"

>>       : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a)

>>       : [new1] "r"(d), [new2] "r"(e));

>> }

>> ===

>> 

>> Is this the same bug that Jeff reported?

>> 

> Yes, it looks similar (reg pair and casp).  I did know it was a kernel.  I only had a preprocessed file slub.i.

> 

> The reduced case is actually wrong.  The constraints +&r does not guarantee that b and c will be in the subsequent regs.  


Right, this was reduction artifact.

> In the original test case b and c were also local *register* variables (x0 and x1).  Generally speaking even this does no guarantee (according to the documentation) that the original variable values will be in subsequent regs for casp.


I don't follow.  Do you mean that in the below testcase it's not guaranteed that casp will get its first two arguments in x0 and x1?  (If so, why?)
===
void *a;
long b, c;
void d(void) {
  typeof(0) e=0;
  register long x0 asm ("x0") = b;
  register long x1 asm ("x1") = c;
  asm("	casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n"
      : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a)
      : [new1] "r"(d), [new2] "r"(e));
}
===

IIRC, there is plenty of syscall code in glibc that relies on asms getting variables in "right" registers.

>  But as people assume such (undocumented) semantics, we should maintain this.


Regards,

--
Maxim Kuvyrkov
www.linaro.org
Vladimir Makarov March 26, 2019, 2:45 p.m. | #4
On 3/26/19 8:35 AM, Maxim Kuvyrkov wrote:
>

> I don't follow.  Do you mean that in the below testcase it's not guaranteed that casp will get its first two arguments in x0 and x1?  (If so, why?)

Sorry for not to be clear.  With my first patch only, it was not 
guaranteed for some complicated code cases.  With the additional patch 
it is guaranteed as it was before.
> ===

> void *a;

> long b, c;

> void d(void) {

>    typeof(0) e=0;

>    register long x0 asm ("x0") = b;

>    register long x1 asm ("x1") = c;

>    asm("	casp\t%[old1], %[old2], %[new1], %[new2], %[v]\n"

>        : [old1] "+&r"(b), [old2] "+&r"(c), [v] "+Q"(a)

>        : [new1] "r"(d), [new2] "r"(e));

> }

>

>

>

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 269923)
+++ ChangeLog	(working copy)
@@ -1,3 +1,10 @@ 
+2019-03-25  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR rtl-optimization/89676
+	* lra-constraints.c (curr_insn_transform): Do match reload for
+	early clobbers when the match was successful only for different
+	registers.
+
 2019-03-25  Martin Sebor  <msebor@redhat.com>
 
 	* doc/extend.texi (Common Type Attributes): Document vector_size.
Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 269878)
+++ lra-constraints.c	(working copy)
@@ -4259,15 +4259,27 @@  curr_insn_transform (bool check_only_p)
 	  else if (goal_alt_matched[i][0] != -1
 		   && curr_static_id->operand[i].type == OP_OUT
 		   && (curr_static_id->operand_alternative
-		       [goal_alt_number * n_operands + i].earlyclobber))
+		       [goal_alt_number * n_operands + i].earlyclobber)
+		   && REG_P (op))
 	    {
-	      /* Generate reloads for output and matched inputs.  This
-		 is the easiest way to avoid creation of non-existing
-		 conflicts in lra-lives.c.  */
-	      match_reload (i, goal_alt_matched[i], outputs, goal_alt[i], &before,
-			    &after, TRUE);
-	      outputs[n_outputs++] = i;
-	      outputs[n_outputs] = -1;
+	      for (j = 0; goal_alt_matched[i][j] != -1; j++)
+		{
+		  rtx op2 = *curr_id->operand_loc[goal_alt_matched[i][j]];
+		  
+		  if (REG_P (op2) && REGNO (op) != REGNO (op2))
+		    break;
+		}
+	      if (goal_alt_matched[i][j] != -1)
+		{
+		  /* Generate reloads for different output and matched
+		     input registers.  This is the easiest way to avoid
+		     creation of non-existing register conflicts in
+		     lra-lives.c.  */
+		  match_reload (i, goal_alt_matched[i], outputs, goal_alt[i], &before,
+				&after, TRUE);
+		  outputs[n_outputs++] = i;
+		  outputs[n_outputs] = -1;
+		}
 	      continue;
 	    }
 	  else