Patchwork Handle target specific memory models in C frontend

login
register
mail settings
Submitter Andi Kleen
Date Nov. 9, 2012, 3:03 p.m.
Message ID <1352473427-11967-1-git-send-email-andi@firstfloor.org>
Download mbox | patch
Permalink /patch/198095/
State New
Headers show

Comments

Andi Kleen - Nov. 9, 2012, 3:03 p.m.
From: Andi Kleen <ak@linux.intel.com>

get_atomic_generic_size would error out for
__atomic_store(...,__ATOMIC_HLE_RELEASE)

Just mask it out. All the memory orders are checked completely
in builtins.c anyways.

I'm not sure what that check is for, it could be removed in theory.

Passed bootstrap and test suite on x86-64

gcc/c-family/:
2012-11-09  Andi Kleen  <ak@linux.intel.com>

	PR55139
	* c-common.c (get_atomic_generic_size): Mask with
        MEMMODEL_MASK
---
 gcc/c-family/c-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Richard Henderson - Nov. 9, 2012, 3:08 p.m.
On 2012-11-09 07:03, Andi Kleen wrote:
> 	PR55139
> 	* c-common.c (get_atomic_generic_size): Mask with
>         MEMMODEL_MASK

Ok.


r~
Jakub Jelinek - Nov. 9, 2012, 3:18 p.m.
Hi!

Just nits.

On Fri, Nov 09, 2012 at 04:03:47PM +0100, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> get_atomic_generic_size would error out for
> __atomic_store(...,__ATOMIC_HLE_RELEASE)
> 
> Just mask it out. All the memory orders are checked completely
> in builtins.c anyways.
> 
> I'm not sure what that check is for, it could be removed in theory.

I think it actually should be removed, given that e.g.
int i, j;
void
foo (void)
{
  __atomic_store (&i, &j, 12);
}
warns twice for the same thing, when it should only warn once.
Richard?

> Passed bootstrap and test suite on x86-64
> 
> gcc/c-family/:
> 2012-11-09  Andi Kleen  <ak@linux.intel.com>
> 
> 	PR55139

That should be PR target/55139

> 	* c-common.c (get_atomic_generic_size): Mask with
>         MEMMODEL_MASK

Dot after MEMMODEL_MASK and don't wrap it on next line.

> ---
>  gcc/c-family/c-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index c3a6465..7828d21 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -10097,7 +10097,7 @@ get_atomic_generic_size (location_t loc, tree function, VEC(tree,gc) *params)
>        if (TREE_CODE (p) == INTEGER_CST)
>          {
>  	  int i = tree_low_cst (p, 1);
> -	  if (i < 0 || i >= MEMMODEL_LAST)
> +	  if (i < 0 || (i & MEMMODEL_MASK) >= MEMMODEL_LAST)
>  	    {
>  	      warning_at (loc, OPT_Winvalid_memory_model,
>  			  "invalid memory model argument %d of %qE", x + 1,

If that whole loop doesn't get removed, there are some formatting issues in
it:
for (x = n_param - n_model ; x < n_param; x++)
shouldn't have space before ; and 
            error_at (loc, "non-integer memory model argument %d of %qE", x + 1,
                   function);
doesn't correctly horizontally aling the second line.

	Jakub
Richard Henderson - Nov. 9, 2012, 4:43 p.m.
On 2012-11-09 07:18, Jakub Jelinek wrote:
> I think it actually should be removed, given that e.g.
> int i, j;
> void
> foo (void)
> {
>   __atomic_store (&i, &j, 12);
> }
> warns twice for the same thing, when it should only warn once.
> Richard?

Makes sense.


r~
Andi Kleen - Nov. 9, 2012, 4:54 p.m.
Richard Henderson <rth@redhat.com> writes:

> On 2012-11-09 07:18, Jakub Jelinek wrote:
>> I think it actually should be removed, given that e.g.
>> int i, j;
>> void
>> foo (void)
>> {
>>   __atomic_store (&i, &j, 12);
>> }
>> warns twice for the same thing, when it should only warn once.
>> Richard?
>
> Makes sense.

Ok, I'll remove it completely and fix the ChangeLog.

Thanks.

-Andi
Andi Kleen - Aug. 10, 2013, 7:40 p.m.
On Fri, Nov 09, 2012 at 07:08:07AM -0800, Richard Henderson wrote:
> On 2012-11-09 07:03, Andi Kleen wrote:
> > 	PR55139
> > 	* c-common.c (get_atomic_generic_size): Mask with
> >         MEMMODEL_MASK
> 
> Ok.

I would like to backport this patch to 4.8. Ok?

-Andi
Andi Kleen - Sept. 8, 2013, 7:41 p.m.
Andi Kleen <andi@firstfloor.org> writes:

> On Fri, Nov 09, 2012 at 07:08:07AM -0800, Richard Henderson wrote:
>> On 2012-11-09 07:03, Andi Kleen wrote:
>> > 	PR55139
>> > 	* c-common.c (get_atomic_generic_size): Mask with
>> >         MEMMODEL_MASK
>> 
>> Ok.
>
> I would like to backport this patch to 4.8. Ok?
>


Ping?
Richard Henderson - Sept. 9, 2013, 6:33 p.m.
On 08/10/2013 12:40 PM, Andi Kleen wrote:
> On Fri, Nov 09, 2012 at 07:08:07AM -0800, Richard Henderson wrote:
>> On 2012-11-09 07:03, Andi Kleen wrote:
>>> 	PR55139
>>> 	* c-common.c (get_atomic_generic_size): Mask with
>>>         MEMMODEL_MASK
>>
>> Ok.
> 
> I would like to backport this patch to 4.8. Ok?

Ok.


r~

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index c3a6465..7828d21 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -10097,7 +10097,7 @@  get_atomic_generic_size (location_t loc, tree function, VEC(tree,gc) *params)
       if (TREE_CODE (p) == INTEGER_CST)
         {
 	  int i = tree_low_cst (p, 1);
-	  if (i < 0 || i >= MEMMODEL_LAST)
+	  if (i < 0 || (i & MEMMODEL_MASK) >= MEMMODEL_LAST)
 	    {
 	      warning_at (loc, OPT_Winvalid_memory_model,
 			  "invalid memory model argument %d of %qE", x + 1,