[C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Paolo Carlini]

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

Hi,

while working on cp_build_binary_op I noticed that the testsuite wasn't 
exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even more 
the code handling those tree codes seemed completely unused. Turned out 
that the C front-end doesn't handle those tree codes at all: I'm coming 
to the conclusion that the C++ front-end bits too are now obsolete and 
may be removed, because only the middle-end generates those codes in 
order to implement optimizations. Anything I'm missing? Any additional 
testing? Tested x86_64-linux.

Thanks, Paolo.

///////////////////


[-- Attachment #2: CL_rotate --]
[-- Type: text/plain, Size: 309 bytes --]

2019-10-10  Paolo Carlini  <paolo.carlini@oracle.com>

	* typeck.c (cp_build_binary_op): Do not handle RROTATE_EXPR and
	LROTATE_EXPR.
	* constexpr.c (cxx_eval_constant_expression): Likewise.
	(potential_constant_expression_1): Likewise.
	* cp-gimplify.c (cp_fold): Likewise.
	* pt.c (tsubst_copy): Likewise.

[-- Attachment #3: patch_rotate --]
[-- Type: text/plain, Size: 2998 bytes --]

Index: constexpr.c
===================================================================
--- constexpr.c	(revision 276805)
+++ constexpr.c	(working copy)
@@ -5115,8 +5115,6 @@ cxx_eval_constant_expression (const constexpr_ctx
     case MAX_EXPR:
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
-    case LROTATE_EXPR:
-    case RROTATE_EXPR:
     case BIT_IOR_EXPR:
     case BIT_XOR_EXPR:
     case BIT_AND_EXPR:
@@ -7103,8 +7101,6 @@ potential_constant_expression_1 (tree t, bool want
     case MAX_EXPR:
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
-    case LROTATE_EXPR:
-    case RROTATE_EXPR:
     case BIT_IOR_EXPR:
     case BIT_XOR_EXPR:
     case BIT_AND_EXPR:
Index: cp-gimplify.c
===================================================================
--- cp-gimplify.c	(revision 276805)
+++ cp-gimplify.c	(working copy)
@@ -2468,8 +2468,6 @@ cp_fold (tree x)
     case MAX_EXPR:
     case LSHIFT_EXPR:
     case RSHIFT_EXPR:
-    case LROTATE_EXPR:
-    case RROTATE_EXPR:
     case BIT_AND_EXPR:
     case BIT_IOR_EXPR:
     case BIT_XOR_EXPR:
Index: pt.c
===================================================================
--- pt.c	(revision 276805)
+++ pt.c	(working copy)
@@ -16308,8 +16308,6 @@ tsubst_copy (tree t, tree args, tsubst_flags_t com
     case TRUTH_OR_EXPR:
     case RSHIFT_EXPR:
     case LSHIFT_EXPR:
-    case RROTATE_EXPR:
-    case LROTATE_EXPR:
     case EQ_EXPR:
     case NE_EXPR:
     case MAX_EXPR:
@@ -18913,8 +18911,6 @@ tsubst_copy_and_build (tree t,
     case TRUTH_OR_EXPR:
     case RSHIFT_EXPR:
     case LSHIFT_EXPR:
-    case RROTATE_EXPR:
-    case LROTATE_EXPR:
     case EQ_EXPR:
     case NE_EXPR:
     case MAX_EXPR:
Index: typeck.c
===================================================================
--- typeck.c	(revision 276805)
+++ typeck.c	(working copy)
@@ -4896,35 +4896,6 @@ cp_build_binary_op (const op_location_t &location,
 	}
       break;
 
-    case RROTATE_EXPR:
-    case LROTATE_EXPR:
-      if (code0 == INTEGER_TYPE && code1 == INTEGER_TYPE)
-	{
-	  result_type = type0;
-	  if (TREE_CODE (op1) == INTEGER_CST)
-	    {
-	      if (tree_int_cst_lt (op1, integer_zero_node))
-		{
-		  if (complain & tf_warning)
-		    warning (0, (code == LROTATE_EXPR)
-			          ? G_("left rotate count is negative")
-   			          : G_("right rotate count is negative"));
-		}
-	      else if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
-		{
-		  if (complain & tf_warning)
-		    warning (0, (code == LROTATE_EXPR) 
-                                  ? G_("left rotate count >= width of type")
-                                  : G_("right rotate count >= width of type"));
-		}
-	    }
-	  /* Convert the shift-count to an integer, regardless of
-	     size of value being shifted.  */
-	  if (TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
-	    op1 = cp_convert (integer_type_node, op1, complain);
-	}
-      break;
-
     case EQ_EXPR:
     case NE_EXPR:
       if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE)

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Jason Merrill]

On 10/10/19 12:12 PM, Paolo Carlini wrote:
> Hi,
> 
> while working on cp_build_binary_op I noticed that the testsuite wasn't 
> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even more 
> the code handling those tree codes seemed completely unused. Turned out 
> that the C front-end doesn't handle those tree codes at all: I'm coming 
> to the conclusion that the C++ front-end bits too are now obsolete and 
> may be removed, because only the middle-end generates those codes in 
> order to implement optimizations. Anything I'm missing? Any additional 
> testing? Tested x86_64-linux.
> 
> Thanks, Paolo.
> 
> ///////////////////
> 
OK.

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Jakub Jelinek]

On Thu, Oct 10, 2019 at 06:12:02PM +0200, Paolo Carlini wrote:
> while working on cp_build_binary_op I noticed that the testsuite wasn't
> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even more the
> code handling those tree codes seemed completely unused. Turned out that the
> C front-end doesn't handle those tree codes at all: I'm coming to the
> conclusion that the C++ front-end bits too are now obsolete and may be
> removed, because only the middle-end generates those codes in order to
> implement optimizations. Anything I'm missing? Any additional testing?

I guess it depends on where.
fold_binary_loc certainly has code to create {L,R}ROTATE_EXPR,
just look at
unsigned foo (unsigned x)
{
  return (x << 3) + (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - 3));
}

unsigned bar (unsigned x, unsigned y)
{
  return (x << y) | (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - y));
}
and the *.original dump.
The cp_build_binary_op case is unlikely to ever trigger, unless we'd rerun
it on cp_folded trees.
cxx_eval_constant_expression is unlikely, because recently we've switched to
performing constexpr evaluation on pre-cp_folded bodies, not sure if we
never encounter folded trees though.
cp_fold itself depends on whether we ever reprocess the already folded
trees, I'd be afraid we could.
pt.c again unlikely, we should be cp_folding only later on.

	Jakub

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Jason Merrill]

On 10/10/19 1:53 PM, Jakub Jelinek wrote:
> On Thu, Oct 10, 2019 at 06:12:02PM +0200, Paolo Carlini wrote:
>> while working on cp_build_binary_op I noticed that the testsuite wasn't
>> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even more the
>> code handling those tree codes seemed completely unused. Turned out that the
>> C front-end doesn't handle those tree codes at all: I'm coming to the
>> conclusion that the C++ front-end bits too are now obsolete and may be
>> removed, because only the middle-end generates those codes in order to
>> implement optimizations. Anything I'm missing? Any additional testing?
> 
> I guess it depends on where.
> fold_binary_loc certainly has code to create {L,R}ROTATE_EXPR,
> just look at
> unsigned foo (unsigned x)
> {
>    return (x << 3) + (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - 3));
> }
> 
> unsigned bar (unsigned x, unsigned y)
> {
>    return (x << y) | (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - y));
> }
> and the *.original dump.
> The cp_build_binary_op case is unlikely to ever trigger, unless we'd rerun
> it on cp_folded trees.
> cxx_eval_constant_expression is unlikely, because recently we've switched to
> performing constexpr evaluation on pre-cp_folded bodies, not sure if we
> never encounter folded trees though.
> cp_fold itself depends on whether we ever reprocess the already folded
> trees, I'd be afraid we could.

True, and the failure mode there is silent.  Let's leave the codes in 
that switch.

Jason

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Paolo Carlini]

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

Hi,

On 10/10/19 19:57, Jason Merrill wrote:
> On 10/10/19 1:53 PM, Jakub Jelinek wrote:
>> On Thu, Oct 10, 2019 at 06:12:02PM +0200, Paolo Carlini wrote:
>>> while working on cp_build_binary_op I noticed that the testsuite wasn't
>>> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even 
>>> more the
>>> code handling those tree codes seemed completely unused. Turned out 
>>> that the
>>> C front-end doesn't handle those tree codes at all: I'm coming to the
>>> conclusion that the C++ front-end bits too are now obsolete and may be
>>> removed, because only the middle-end generates those codes in order to
>>> implement optimizations. Anything I'm missing? Any additional testing?
>>
>> I guess it depends on where.
>> fold_binary_loc certainly has code to create {L,R}ROTATE_EXPR,
>> just look at
>> unsigned foo (unsigned x)
>> {
>>    return (x << 3) + (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - 3));
>> }
>>
>> unsigned bar (unsigned x, unsigned y)
>> {
>>    return (x << y) | (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - y));
>> }
>> and the *.original dump.
>> The cp_build_binary_op case is unlikely to ever trigger, unless we'd 
>> rerun
>> it on cp_folded trees.
>> cxx_eval_constant_expression is unlikely, because recently we've 
>> switched to
>> performing constexpr evaluation on pre-cp_folded bodies, not sure if we
>> never encounter folded trees though.
>> cp_fold itself depends on whether we ever reprocess the already folded
>> trees, I'd be afraid we could.
>
> True, and the failure mode there is silent.  Let's leave the codes in 
> that switch.

Ok, thanks Jason and Jakub for the additional information.

While I give this more thought and maybe manage to come up with a 
testcase triggering the warning, shall we simply pass the location to 
those two warnings too - cannot hurt, AFAICS?

Thanks, Paolo.

////////////////////




[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1147 bytes --]

Index: typeck.c
===================================================================
--- typeck.c	(revision 276845)
+++ typeck.c	(working copy)
@@ -4906,16 +4906,16 @@ cp_build_binary_op (const op_location_t &location,
 	      if (tree_int_cst_lt (op1, integer_zero_node))
 		{
 		  if (complain & tf_warning)
-		    warning (0, (code == LROTATE_EXPR)
-			          ? G_("left rotate count is negative")
-   			          : G_("right rotate count is negative"));
+		    warning_at (location, 0, (code == LROTATE_EXPR)
+				? G_("left rotate count is negative")
+				: G_("right rotate count is negative"));
 		}
 	      else if (compare_tree_int (op1, TYPE_PRECISION (type0)) >= 0)
 		{
 		  if (complain & tf_warning)
-		    warning (0, (code == LROTATE_EXPR) 
-                                  ? G_("left rotate count >= width of type")
-                                  : G_("right rotate count >= width of type"));
+		    warning_at (location, 0, (code == LROTATE_EXPR) 
+				? G_("left rotate count >= width of type")
+				: G_("right rotate count >= width of type"));
 		}
 	    }
 	  /* Convert the shift-count to an integer, regardless of

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Jason Merrill]

On 10/10/19 2:33 PM, Paolo Carlini wrote:
> Hi,
> 
> On 10/10/19 19:57, Jason Merrill wrote:
>> On 10/10/19 1:53 PM, Jakub Jelinek wrote:
>>> On Thu, Oct 10, 2019 at 06:12:02PM +0200, Paolo Carlini wrote:
>>>> while working on cp_build_binary_op I noticed that the testsuite wasn't
>>>> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even 
>>>> more the
>>>> code handling those tree codes seemed completely unused. Turned out 
>>>> that the
>>>> C front-end doesn't handle those tree codes at all: I'm coming to the
>>>> conclusion that the C++ front-end bits too are now obsolete and may be
>>>> removed, because only the middle-end generates those codes in order to
>>>> implement optimizations. Anything I'm missing? Any additional testing?
>>>
>>> I guess it depends on where.
>>> fold_binary_loc certainly has code to create {L,R}ROTATE_EXPR,
>>> just look at
>>> unsigned foo (unsigned x)
>>> {
>>>    return (x << 3) + (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - 3));
>>> }
>>>
>>> unsigned bar (unsigned x, unsigned y)
>>> {
>>>    return (x << y) | (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - y));
>>> }
>>> and the *.original dump.
>>> The cp_build_binary_op case is unlikely to ever trigger, unless we'd 
>>> rerun
>>> it on cp_folded trees.
>>> cxx_eval_constant_expression is unlikely, because recently we've 
>>> switched to
>>> performing constexpr evaluation on pre-cp_folded bodies, not sure if we
>>> never encounter folded trees though.
>>> cp_fold itself depends on whether we ever reprocess the already folded
>>> trees, I'd be afraid we could.
>>
>> True, and the failure mode there is silent.  Let's leave the codes in 
>> that switch.
> 
> Ok, thanks Jason and Jakub for the additional information.
> 
> While I give this more thought and maybe manage to come up with a 
> testcase triggering the warning, shall we simply pass the location to 
> those two warnings too - cannot hurt, AFAICS?

I meant let's omit the changes to cp_fold, the rest of the patch is 
still OK.

Jason

Re: [C++ Patch] Remove RROTATE_EXPR and LROTATE_EXPR handling

[Paolo Carlini]

Hi,

On 11/10/19 18:25, Jason Merrill wrote:
> On 10/10/19 2:33 PM, Paolo Carlini wrote:
>> Hi,
>>
>> On 10/10/19 19:57, Jason Merrill wrote:
>>> On 10/10/19 1:53 PM, Jakub Jelinek wrote:
>>>> On Thu, Oct 10, 2019 at 06:12:02PM +0200, Paolo Carlini wrote:
>>>>> while working on cp_build_binary_op I noticed that the testsuite 
>>>>> wasn't
>>>>> exercising the warnings in case RROTATE_EXPR / LROTATE_EXPR, even 
>>>>> more the
>>>>> code handling those tree codes seemed completely unused. Turned 
>>>>> out that the
>>>>> C front-end doesn't handle those tree codes at all: I'm coming to the
>>>>> conclusion that the C++ front-end bits too are now obsolete and 
>>>>> may be
>>>>> removed, because only the middle-end generates those codes in 
>>>>> order to
>>>>> implement optimizations. Anything I'm missing? Any additional 
>>>>> testing?
>>>>
>>>> I guess it depends on where.
>>>> fold_binary_loc certainly has code to create {L,R}ROTATE_EXPR,
>>>> just look at
>>>> unsigned foo (unsigned x)
>>>> {
>>>>    return (x << 3) + (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - 3));
>>>> }
>>>>
>>>> unsigned bar (unsigned x, unsigned y)
>>>> {
>>>>    return (x << y) | (x >> (__SIZEOF_INT__ * __CHAR_BIT__ - y));
>>>> }
>>>> and the *.original dump.
>>>> The cp_build_binary_op case is unlikely to ever trigger, unless 
>>>> we'd rerun
>>>> it on cp_folded trees.
>>>> cxx_eval_constant_expression is unlikely, because recently we've 
>>>> switched to
>>>> performing constexpr evaluation on pre-cp_folded bodies, not sure 
>>>> if we
>>>> never encounter folded trees though.
>>>> cp_fold itself depends on whether we ever reprocess the already folded
>>>> trees, I'd be afraid we could.
>>>
>>> True, and the failure mode there is silent.  Let's leave the codes 
>>> in that switch.
>>
>> Ok, thanks Jason and Jakub for the additional information.
>>
>> While I give this more thought and maybe manage to come up with a 
>> testcase triggering the warning, shall we simply pass the location to 
>> those two warnings too - cannot hurt, AFAICS?
>
> I meant let's omit the changes to cp_fold, the rest of the patch is 
> still OK.

Nice, thanks, I'll do that.

Paolo.