diff mbox

Fix rtx_equal_p and similar predicates on ASM_OPERANDS/ASM_INPUT (PR rtl-optimization/46865)

Message ID 4D022224.4000407@codesourcery.com
State New
Headers show

Commit Message

Chung-Lin Tang Dec. 10, 2010, 12:50 p.m. UTC 
On 2010/12/10 06:06, Jakub Jelinek wrote:
> The last operand of ASM_OPERANDS and ASM_INPUT is a RTL locator,
> for which we unfortunately don't guarantee uniqueness.  One has
> to call locator_eq to actually compare them.
> On the testcases below, without -save-temps the locators are equal
> while without them some other location_t got a locator in between.

Actually, I think the issue is not just about comparing locators, but 
rather: why do you need locator equality for ASM_INPUT/ASM_OPERANDS to 
be equivalent? The checking of the other fields should be sufficient. 
This way for the testcase, the non-macro version would then also be 
combined into a single asm.

I was about to submit this patch, which simply skips the ending 'i' 
field for ASM_INPUT/ASM_OPERANDS when checking equivalence. Bootstrapped 
and tested on i686 and x86_64; tested on ARM-Linux with QEMU.

Thanks,
Chung-Lin

2010-12-10  Chung-Lin Tang  <cltang@codesourcery.com>

	PR rtl-optimization/46865
	* rtl.c (rtx_equal_p_cb): Skip location operand when comparing
	for equivalence.
	(rtx_equal_p): Same.
	* jump.c (rtx_renumbered_equal_p): Same.

Comments

Jakub Jelinek Dec. 10, 2010, 12:56 p.m. UTC | #1 
On Fri, Dec 10, 2010 at 08:50:44PM +0800, Chung-Lin Tang wrote:
> On 2010/12/10 06:06, Jakub Jelinek wrote:
> >The last operand of ASM_OPERANDS and ASM_INPUT is a RTL locator,
> >for which we unfortunately don't guarantee uniqueness.  One has
> >to call locator_eq to actually compare them.
> >On the testcases below, without -save-temps the locators are equal
> >while without them some other location_t got a locator in between.
> 
> Actually, I think the issue is not just about comparing locators,
> but rather: why do you need locator equality for
> ASM_INPUT/ASM_OPERANDS to be equivalent? The checking of the other

Because then you regress in debug info quality, we emit the location
with .loc before the asm.  Though, for -O0 we hopefully don't perform
crossjumping and similar optimizations, and for -O1+ the code quality is
more important than debug info quality.

If we'd want to ignore it, the right fix wouldn't be your patch,
but just making that operand use 0 format instead of i and adjust
print-rtl.c etc.

	Jakub
Chung-Lin Tang Dec. 10, 2010, 1:13 p.m. UTC | #2 
Jakub Jelinek wrote:
>> Actually, I think the issue is not just about comparing locators,
>> >  but rather: why do you need locator equality for
>> >  ASM_INPUT/ASM_OPERANDS to be equivalent? The checking of the other
> Because then you regress in debug info quality, we emit the location
> with .loc before the asm.  Though, for -O0 we hopefully don't perform
> crossjumping and similar optimizations, and for -O1+ the code quality is
> more important than debug info quality.
>
> If we'd want to ignore it, the right fix wouldn't be your patch,
> but just making that operand use 0 format instead of i and adjust
> print-rtl.c etc.

I see your point.
However, to adjust the equivalence criteria depending on optimization 
level (as you suggested), changing to '0' format does not make it easier 
:)   I would still suggest using code to achieve that.

CL
diff mbox

Patch

Index: rtl.c
===================================================================
--- rtl.c	(revision 167678)
+++ rtl.c	(working copy)
@@ -360,6 +360,7 @@ 
   int j;
   enum rtx_code code;
   const char *fmt;
+  int fmt_end;
   rtx nx, ny;
 
   if (x == y)
@@ -419,7 +420,10 @@ 
      fail to match, return 0 for the whole thing.  */
 
   fmt = GET_RTX_FORMAT (code);
-  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
+  fmt_end = (GET_RTX_LENGTH (code)
+	     - (code == ASM_INPUT || code == ASM_OPERANDS ? 2 : 1));
+
+  for (i = fmt_end; i >= 0; i--)
     {
       switch (fmt[i])
 	{
@@ -490,6 +494,7 @@ 
   int j;
   enum rtx_code code;
   const char *fmt;
+  int fmt_end;
 
   if (x == y)
     return 1;
@@ -543,7 +548,10 @@ 
      fail to match, return 0 for the whole thing.  */
 
   fmt = GET_RTX_FORMAT (code);
-  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
+  fmt_end = (GET_RTX_LENGTH (code)
+	     - (code == ASM_INPUT || code == ASM_OPERANDS ? 2 : 1));
+
+  for (i = fmt_end; i >= 0; i--)
     {
       switch (fmt[i])
 	{
Index: jump.c
===================================================================
--- jump.c	(revision 167678)
+++ jump.c	(working copy)
@@ -1584,6 +1584,7 @@ 
   int i;
   const enum rtx_code code = GET_CODE (x);
   const char *fmt;
+  int fmt_end;
 
   if (x == y)
     return 1;
@@ -1715,7 +1716,10 @@ 
      fail to match, return 0 for the whole things.  */
 
   fmt = GET_RTX_FORMAT (code);
-  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
+  fmt_end = (GET_RTX_LENGTH (code)
+	     - (code == ASM_INPUT || code == ASM_OPERANDS ? 2 : 1));
+
+  for (i = fmt_end; i >= 0; i--)
     {
       int j;
       switch (fmt[i])