[C++] Remove RROTATE_EXPR and LROTATE_EXPR handling

Message ID 6e15b734-8836-07e3-ce58-2b10086a41f7@oracle.com
State New
Headers show
Series
  • [C++] Remove RROTATE_EXPR and LROTATE_EXPR handling
Related show

Commit Message

Paolo Carlini Oct. 10, 2019, 4:12 p.m.
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.

///////////////////
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.

Comments

Jason Merrill Oct. 10, 2019, 5:42 p.m. | #1
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.
Jakub Jelinek Oct. 10, 2019, 5:53 p.m. | #2
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
Jason Merrill Oct. 10, 2019, 5:57 p.m. | #3
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
Paolo Carlini Oct. 10, 2019, 6:33 p.m. | #4
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.

////////////////////
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
Jason Merrill Oct. 11, 2019, 4:25 p.m. | #5
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
Paolo Carlini Oct. 11, 2019, 4:38 p.m. | #6
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.

Patch

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)