[PATCH, generic] Fix for define_subst

[PATCH, generic] Fix for define_subst

[Kirill Yukhin]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

Hello,
Here is copy-and-paste from issue raised by Ian (in the bottom).

Fix is attached.
ChangeLog entry:
2012-11-22  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>

        * gensupport.c (add_c_test): Check if expr isn't NULL.

Is it ok for trunk?

=== CUT HERE ===
It looks like the recent VEC rewrite might have interacted badly with
this patch.

I am getting a segfault in genconditions.c when add_c_test is called
for "desc".

I think it is because the condition is not within a vector.


Example of "desc" for working pattern:

(define_insn ("*zero_extendqihi2_aarch64")
     [
        (set (match_operand:HI 0 ("register_operand") ("=r,r"))
            (zero_extend:HI (match_operand:QI 1 ("nonimmediate_operand")
("r,m"))))
    ] ("") ("@
   uxtb\t%w0, %w1
   ldrb\t%w0, %1")
     [
        (set_attr ("v8type") ("extend,load1"))
        (set_attr ("mode") ("HI"))
    ])


Example of "desc" for broken one that uses define_subst:

(define_insn ("*addsi3_aarch64_noextend")
     [
        (set (match_operand:SI 0 ("register_operand") ("=rk,rk,rk"))
            (plus:SI (match_operand:SI 1 ("register_operand") ("%rk,rk,rk"))
                (match_operand:SI 2 ("aarch64_plus_operand") ("I,r,J"))))
    ] "" ("@
  add\t%w0, %w1, %2
  add\t%w0, %w1, %w2
  sub\t%w0, %w1, #%n2")
     [
        (set_attr ("v8type") ("alu"))
        (set_attr ("mode") ("SI"))
    ])


Note that there are no brackets around the "" condition.  That's the issue,
I
think.

=== CUT HERE ===

Thanks, K

[-- Attachment #2: add_c_test.patch --]
[-- Type: application/octet-stream, Size: 315 bytes --]

diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index 4aa11bc..1bd166e 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -2668,7 +2668,7 @@ add_c_test (const char *expr, int value)
 {
   struct c_test *test;
 
-  if (expr[0] == 0)
+  if (!expr || expr[0] == 0)
     return;
 
   test = XNEW (struct c_test);

Re: [PATCH, generic] Fix for define_subst

[Kirill Yukhin]

Guys, this is a ping.

Thanks, K

On Thu, Nov 22, 2012 at 9:36 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> Hello,
> Here is copy-and-paste from issue raised by Ian (in the bottom).
>
> Fix is attached.
> ChangeLog entry:
> 2012-11-22  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>
>
>         * gensupport.c (add_c_test): Check if expr isn't NULL.
>
> Is it ok for trunk?
>
> === CUT HERE ===
> It looks like the recent VEC rewrite might have interacted badly with
> this patch.
>
> I am getting a segfault in genconditions.c when add_c_test is called
> for "desc".
>
> I think it is because the condition is not within a vector.
>
>
> Example of "desc" for working pattern:
>
> (define_insn ("*zero_extendqihi2_aarch64")
>      [
>         (set (match_operand:HI 0 ("register_operand") ("=r,r"))
>             (zero_extend:HI (match_operand:QI 1 ("nonimmediate_operand")
> ("r,m"))))
>     ] ("") ("@
>    uxtb\t%w0, %w1
>    ldrb\t%w0, %1")
>      [
>         (set_attr ("v8type") ("extend,load1"))
>         (set_attr ("mode") ("HI"))
>     ])
>
>
> Example of "desc" for broken one that uses define_subst:
>
> (define_insn ("*addsi3_aarch64_noextend")
>      [
>         (set (match_operand:SI 0 ("register_operand") ("=rk,rk,rk"))
>             (plus:SI (match_operand:SI 1 ("register_operand") ("%rk,rk,rk"))
>                 (match_operand:SI 2 ("aarch64_plus_operand") ("I,r,J"))))
>     ] "" ("@
>   add\t%w0, %w1, %2
>   add\t%w0, %w1, %w2
>   sub\t%w0, %w1, #%n2")
>      [
>         (set_attr ("v8type") ("alu"))
>         (set_attr ("mode") ("SI"))
>     ])
>
>
> Note that there are no brackets around the "" condition.  That's the issue,
> I
> think.
>
> === CUT HERE ===
>
> Thanks, K

Re: [PATCH, generic] Fix for define_subst

[Richard Henderson]

On 11/22/2012 09:36 AM, Kirill Yukhin wrote:
> @@ -2668,7 +2668,7 @@ add_c_test (const char *expr, int value)
>  {
>    struct c_test *test;
>  
> -  if (expr[0] == 0)
> +  if (!expr || expr[0] == 0)
>      return;

This looks like it's working around a bug elsewhere.


r~

Re: [PATCH, generic] Fix for define_subst

[Michael Zolotukhin]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

Hi Richard,

> This looks like it's working around a bug elsewhere.
Originally, that fail is caused by function join_c_conditions(const
char *cond1, const char *cond2) - if cond1 is "" and cond2 is NULL,
then the function returns cond2, i.e. NULL. Attached patch fixes it -
specifically, with it the function join_c_conditions prefers "" over
NULL.

With this patch there is no need in the change in add_c_test, which
was proposed above, though maybe it's worth having it, as NULL strings
are not prohibited. If we decide to not change add_c_test, then we'd
probably need to remove similar check from function maybe_eval_c_test.
What do you think?

Changelog:
2012-11-28  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>

        * gensupport.c (add_c_test): Check if expr isn't NULL.
        * read-md.c (join_c_conditions): Prefer empty string over NULL.

-- 
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.

[-- Attachment #2: join_c_conditions.patch --]
[-- Type: application/octet-stream, Size: 770 bytes --]

diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index 00290b0..4186f85 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -2668,7 +2668,7 @@ add_c_test (const char *expr, int value)
 {
   struct c_test *test;
 
-  if (expr[0] == 0)
+  if (!expr || expr[0] == 0)
     return;
 
   test = XNEW (struct c_test);
diff --git a/gcc/read-md.c b/gcc/read-md.c
index e5534d7..d229614 100644
--- a/gcc/read-md.c
+++ b/gcc/read-md.c
@@ -194,10 +194,10 @@ join_c_conditions (const char *cond1, const char *cond2)
   const void **entry;
 
   if (cond1 == 0 || cond1[0] == 0)
-    return cond2;
+    return cond2 ? cond2 : cond1;
 
   if (cond2 == 0 || cond2[0] == 0)
-    return cond1;
+    return cond1 ? cond1 : cond2;
 
   if (strcmp (cond1, cond2) == 0)
     return cond1;

Re: [PATCH, generic] Fix for define_subst

[Richard Henderson]

On 11/28/2012 07:55 AM, Michael Zolotukhin wrote:
> Originally, that fail is caused by function join_c_conditions(const
> char *cond1, const char *cond2) - if cond1 is "" and cond2 is NULL

Joining a null condition suggests that's not the original fail.
Where was the null condition created in the first place?


r~

Re: [PATCH, generic] Fix for define_subst

[Michael Zolotukhin]

> Where was the null condition created in the first place?
The reason it's happening is following. Before introduction of
define_subst, function apply_iterators had the following loop:
      condition = NULL;
      FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
        {
          v = iuse->iterator->current_value;
          iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
          condition = join_c_conditions (condition, v->string);
        }
      apply_attribute_uses ();
      x = copy_rtx_for_iterators (original);
      add_condition_to_rtx (x, condition);

This loop always iterated at least once, so 'condition' always became
not-null (though, it could become empty-string ""). So, function
add_condition_to_rtx always had not-null string in the arguments.

With subst-iterators this loop is looking like this:
      condition = NULL;
      FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
        {
          if (iuse->iterator->group == &substs)
            continue;
          v = iuse->iterator->current_value;
          iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
          condition = join_c_conditions (condition, v->string);
        }
So, it's possible that join_c_condition wouldn't be called at all, and
'condition' will remain NULL. Then it goes to add_condition_to_rtx and
we get the fail we've seen.

There is no mistake in such behaviour, but now we should be aware of
possible NULL-string. It should be handled properly, and I see two
places where we could do that - in join_c_conditions or in add_c_tests
and maybe_eval_c_test.

-- 
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.

Re: [PATCH, generic] Fix for define_subst

[Richard Henderson]

On 11/28/2012 09:20 AM, Michael Zolotukhin wrote:
>> Where was the null condition created in the first place?
> The reason it's happening is following. Before introduction of
> define_subst, function apply_iterators had the following loop:
>       condition = NULL;
>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>         {
>           v = iuse->iterator->current_value;
>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>           condition = join_c_conditions (condition, v->string);
>         }
>       apply_attribute_uses ();
>       x = copy_rtx_for_iterators (original);
>       add_condition_to_rtx (x, condition);
> 
> This loop always iterated at least once, so 'condition' always became
> not-null (though, it could become empty-string ""). So, function
> add_condition_to_rtx always had not-null string in the arguments.
> 
> With subst-iterators this loop is looking like this:
>       condition = NULL;
>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>         {
>           if (iuse->iterator->group == &substs)
>             continue;
>           v = iuse->iterator->current_value;
>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>           condition = join_c_conditions (condition, v->string);
>         }
> So, it's possible that join_c_condition wouldn't be called at all, and
> 'condition' will remain NULL. Then it goes to add_condition_to_rtx and
> we get the fail we've seen.
> 
> There is no mistake in such behaviour, but now we should be aware of
> possible NULL-string. It should be handled properly, and I see two
> places where we could do that - in join_c_conditions or in add_c_tests
> and maybe_eval_c_test.
> 

Well, there does seem to be a mistake -- the use of NULL in the first
place.  It seems to me that the easiest fix is

  condition = "";

right at the beginning.


r~

Re: [PATCH, generic] Fix for define_subst

[Michael Zolotukhin]

> Well, there does seem to be a mistake -- the use of NULL in the first
> place.  It seems to me that the easiest fix is
>
>   condition = "";
>
> right at the beginning.
Yep, that'll work too, you're right.

On 28 November 2012 22:36, Richard Henderson <rth@redhat.com> wrote:
> On 11/28/2012 09:20 AM, Michael Zolotukhin wrote:
>>> Where was the null condition created in the first place?
>> The reason it's happening is following. Before introduction of
>> define_subst, function apply_iterators had the following loop:
>>       condition = NULL;
>>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>>         {
>>           v = iuse->iterator->current_value;
>>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>>           condition = join_c_conditions (condition, v->string);
>>         }
>>       apply_attribute_uses ();
>>       x = copy_rtx_for_iterators (original);
>>       add_condition_to_rtx (x, condition);
>>
>> This loop always iterated at least once, so 'condition' always became
>> not-null (though, it could become empty-string ""). So, function
>> add_condition_to_rtx always had not-null string in the arguments.
>>
>> With subst-iterators this loop is looking like this:
>>       condition = NULL;
>>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>>         {
>>           if (iuse->iterator->group == &substs)
>>             continue;
>>           v = iuse->iterator->current_value;
>>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>>           condition = join_c_conditions (condition, v->string);
>>         }
>> So, it's possible that join_c_condition wouldn't be called at all, and
>> 'condition' will remain NULL. Then it goes to add_condition_to_rtx and
>> we get the fail we've seen.
>>
>> There is no mistake in such behaviour, but now we should be aware of
>> possible NULL-string. It should be handled properly, and I see two
>> places where we could do that - in join_c_conditions or in add_c_tests
>> and maybe_eval_c_test.
>>
>
> Well, there does seem to be a mistake -- the use of NULL in the first
> place.  It seems to me that the easiest fix is
>
>   condition = "";
>
> right at the beginning.
>
>
> r~



-- 
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.

Re: [PATCH, generic] Fix for define_subst

[Michael Zolotukhin]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

So, ok for commit this patch?

Changelog:
2012-11-29  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>

        * gensupport.c (maybe_eval_c_test): Remove not-null check for expr.
        * read-rtl.c (apply_iterators): Initialize condition with "" instead
        of NULL.

On 28 November 2012 23:46, Michael Zolotukhin
<michael.v.zolotukhin@gmail.com> wrote:
>> Well, there does seem to be a mistake -- the use of NULL in the first
>> place.  It seems to me that the easiest fix is
>>
>>   condition = "";
>>
>> right at the beginning.
> Yep, that'll work too, you're right.
>
> On 28 November 2012 22:36, Richard Henderson <rth@redhat.com> wrote:
>> On 11/28/2012 09:20 AM, Michael Zolotukhin wrote:
>>>> Where was the null condition created in the first place?
>>> The reason it's happening is following. Before introduction of
>>> define_subst, function apply_iterators had the following loop:
>>>       condition = NULL;
>>>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>>>         {
>>>           v = iuse->iterator->current_value;
>>>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>>>           condition = join_c_conditions (condition, v->string);
>>>         }
>>>       apply_attribute_uses ();
>>>       x = copy_rtx_for_iterators (original);
>>>       add_condition_to_rtx (x, condition);
>>>
>>> This loop always iterated at least once, so 'condition' always became
>>> not-null (though, it could become empty-string ""). So, function
>>> add_condition_to_rtx always had not-null string in the arguments.
>>>
>>> With subst-iterators this loop is looking like this:
>>>       condition = NULL;
>>>       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
>>>         {
>>>           if (iuse->iterator->group == &substs)
>>>             continue;
>>>           v = iuse->iterator->current_value;
>>>           iuse->iterator->group->apply_iterator (iuse->ptr, v->number);
>>>           condition = join_c_conditions (condition, v->string);
>>>         }
>>> So, it's possible that join_c_condition wouldn't be called at all, and
>>> 'condition' will remain NULL. Then it goes to add_condition_to_rtx and
>>> we get the fail we've seen.
>>>
>>> There is no mistake in such behaviour, but now we should be aware of
>>> possible NULL-string. It should be handled properly, and I see two
>>> places where we could do that - in join_c_conditions or in add_c_tests
>>> and maybe_eval_c_test.
>>>
>>
>> Well, there does seem to be a mistake -- the use of NULL in the first
>> place.  It seems to me that the easiest fix is
>>
>>   condition = "";
>>
>> right at the beginning.
>>
>>
>> r~
>
>
>
> --
> ---
> Best regards,
> Michael V. Zolotukhin,
> Software Engineer
> Intel Corporation.



-- 
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.

[-- Attachment #2: null-condition-fix.patch --]
[-- Type: application/octet-stream, Size: 779 bytes --]

diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index 00290b0..e573f64 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -2650,7 +2650,7 @@ maybe_eval_c_test (const char *expr)
   const struct c_test *test;
   struct c_test dummy;
 
-  if (!expr || expr[0] == 0)
+  if (expr[0] == 0)
     return 1;
 
   dummy.expr = expr;
diff --git a/gcc/read-rtl.c b/gcc/read-rtl.c
index 7da12b5..6dd4fc5 100644
--- a/gcc/read-rtl.c
+++ b/gcc/read-rtl.c
@@ -546,7 +546,7 @@ apply_iterators (rtx original, rtx *queue)
     {
       /* Apply the current iterator values.  Accumulate a condition to
 	 say when the resulting rtx can be used.  */
-      condition = NULL;
+      condition = "";
       FOR_EACH_VEC_ELT (iterator_uses, i, iuse)
 	{
 	  if (iuse->iterator->group == &substs)

Re: [PATCH, generic] Fix for define_subst

[Richard Henderson]

On 2012-11-29 00:58, Michael Zolotukhin wrote:
> 2012-11-29  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>
> 
>         * gensupport.c (maybe_eval_c_test): Remove not-null check for expr.
>         * read-rtl.c (apply_iterators): Initialize condition with "" instead
>         of NULL.

Ok.


r~

Re: [PATCH, generic] Fix for define_subst

[Kirill Yukhin]

Hello,
> Ok.
Thanks, checked in: http://gcc.gnu.org/ml/gcc-cvs/2012-11/msg00937.html

Thanks, K