diff mbox

refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

Message ID 5620AED8.4000805@st.com
State New
Headers show

Commit Message

Christian Bruel Oct. 16, 2015, 8:01 a.m. UTC 
Hello,

On 10/12/2015 01:19 PM, Bernd Schmidt wrote:
> On 10/12/2015 12:56 PM, Christian Bruel wrote:
>> yes I see, I was hoping to avoid a new hook, but as you said it seems
>> mandatory for the mere declaration case.
>>
>> Here is one proposal, it defaults to nothing and the ARM implementation
>> does not need to handle the vptr bit setting. so that simplifies a lot
>> the things.
>>
>> The hook is called from rest_of_decl_compilation for mere declarations
>> and allocate_struct_function for definitions.
>
> This looks good to me. I still think we also want your vptr patch.
>
 > Bernd
 >

Here is the "vptr patch".

2 changes to watch for the review :

  - I replaced the DECL_ALIGN checks by FUNCTION_BOUNDARY because there 
is no user align at that time of processing and it's consistent with the 
default TARGET_PTRMEMFUNC_VBIT_LOCATION macro implementation (or 
TARGET_PTRMEMFUNC_VBIT_LOCATION should also be function of DECL_ALIGN)

  - The second chunk from cp/lambda.c did not seem to be needed (?).
Thunks are FUNCTION_TYPE and I don't see why they would need to carry 
the vptr bit information (for METHOD_TYPE from my understanding). Or did 
I miss something here ?

bootstrapped on x86. no c/c++ new failures for
   x86 (ptrmemfunc_vbit_in_pfn, align 8)
   arm (ptrmemfunc_vbit_in_delta, align 16 or 32)
   sh4 (ptrmemfunc_vbit_in_pfn, align 16)

It's a code cleanup/refactoring. No new testcase.

thanks

Comments

Bernd Schmidt Oct. 16, 2015, 9:44 a.m. UTC | #1 
On 10/16/2015 10:01 AM, Christian Bruel wrote:
> -
> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
> -
> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;

This looks like a change in behaviour. You want to use the max of M_M_B 
and the current alignment.


Bernd
Christian Bruel Oct. 16, 2015, 10:48 a.m. UTC | #2 
On 10/16/2015 11:44 AM, Bernd Schmidt wrote:
> On 10/16/2015 10:01 AM, Christian Bruel wrote:
>> -
>> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
>> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
>> -
>> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
>
> This looks like a change in behaviour. You want to use the max of M_M_B
> and the current alignment.
>
 > Bernd
 >

I'm not sure. at each point of the macro, we have the current alignment 
== FUNCTION_BOUNDARY, because we are just returning from the sequence 
build_lang_decl/make_node

so it looks like

   DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))

would be redundant with just

   DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY

did I miss something ?


>
> Bernd
>
Bernd Schmidt Oct. 16, 2015, 10:53 a.m. UTC | #3 
On 10/16/2015 12:48 PM, Christian Bruel wrote:
> I'm not sure. at each point of the macro, we have the current alignment
> == FUNCTION_BOUNDARY, because we are just returning from the sequence
> build_lang_decl/make_node
>
> so it looks like
>
>    DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))
>
> would be redundant with just
>
>    DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY
>
> did I miss something ?

No, I managed to confuse myself. Patch is ok to install next Wednesday 
unless you hear otherwise from a C++ (or Java) maintainer.


Bernd
Christian Bruel Oct. 16, 2015, 10:54 a.m. UTC | #4 
On 10/16/2015 12:48 PM, Christian Bruel wrote:
>
>
> On 10/16/2015 11:44 AM, Bernd Schmidt wrote:
>> On 10/16/2015 10:01 AM, Christian Bruel wrote:
>>> -
>>> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>>> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
>>> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
>>> -
>>> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
>>
>> This looks like a change in behaviour. You want to use the max of M_M_B
>> and the current alignment.
>>
>  > Bernd
>  >
>
> I'm not sure. at each point of the macro, we have the current alignment
> == FUNCTION_BOUNDARY, because we are just returning from the sequence
> build_lang_decl/make_node
>
> so it looks like
>
>    DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))
>
> would be redundant with just
>
>    DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY
>
> did I miss something ?

maybe I realized that it was not clear that the MAX is part of the MMB 
macro that returns MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNITS)


>
>
>>
>> Bernd
>>
diff mbox

Patch

2015-09-29  Christian Bruel  <christian.bruel@st.com>

	* function.h (MINIMUM_METHOD_BOUNDARY): New macro.
	* cp/decl.c (grokfndecl): Set DECL_ALIGN with MINIMUM_METHOD_BOUNDARY.
	* cp/method.c (implicitly_declare_fn): Likewise.
	* cp/lambda.c (maybe_add_lambda_conv_op): Likewise, remove wrong setting.
	* java/class.c (add_method_1): Likewise.



	  
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 228756)
+++ cp/decl.c	(working copy)
@@ -7826,6 +7826,8 @@  grokfndecl (tree ctype,
       parm = build_this_parm (type, quals);
       DECL_CHAIN (parm) = parms;
       parms = parm;
+ 
+      /* Allocate space to hold the vptr bit if needed.  */
+      DECL_ALIGN (decl) = MINIMUM_METHOD_BOUNDARY;
     }
   DECL_ARGUMENTS (decl) = parms;
   for (t = parms; t; t = DECL_CHAIN (t))
@@ -7849,14 +7851,6 @@  grokfndecl (tree ctype,
       break;
     }
 
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && TREE_CODE (type) == METHOD_TYPE
-      && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;
-
   if (friendp
       && TREE_CODE (orig_declarator) == TEMPLATE_ID_EXPR)
     {
Index: cp/lambda.c
===================================================================
--- cp/lambda.c	(revision 228756)
+++ cp/lambda.c	(working copy)
@@ -1006,11 +1006,8 @@  maybe_add_lambda_conv_op (tree type)
   tree convfn = build_lang_decl (FUNCTION_DECL, name, fntype);
   tree fn = convfn;
   DECL_SOURCE_LOCATION (fn) = DECL_SOURCE_LOCATION (callop);
-
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
-
+  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
   SET_OVERLOADED_OPERATOR_CODE (fn, TYPE_EXPR);
   grokclassfn (type, fn, NO_SPECIAL);
   set_linkage_according_to_type (type, fn);
@@ -1042,9 +1039,7 @@  maybe_add_lambda_conv_op (tree type)
   tree statfn = build_lang_decl (FUNCTION_DECL, name, stattype);
   fn = statfn;
   DECL_SOURCE_LOCATION (fn) = DECL_SOURCE_LOCATION (callop);
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
   grokclassfn (type, fn, NO_SPECIAL);
   set_linkage_according_to_type (type, fn);
   rest_of_decl_compilation (fn, toplevel_bindings_p (), at_eof);
Index: cp/method.c
===================================================================
--- cp/method.c	(revision 228756)
+++ cp/method.c	(working copy)
@@ -1849,13 +1849,9 @@  implicitly_declare_fn (special_function_
       DECL_ASSIGNMENT_OPERATOR_P (fn) = 1;
       SET_OVERLOADED_OPERATOR_CODE (fn, NOP_EXPR);
     }
-  
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
+
+  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
 
   /* Create the explicit arguments.  */
   if (rhs_parm_type)
Index: function.h
===================================================================
--- function.h	(revision 228756)
+++ function.h	(working copy)
@@ -537,6 +537,13 @@  do {								\
 #define ASLK_REDUCE_ALIGN 1
 #define ASLK_RECORD_PAD 2
 
+/* If pointers to member functions use the least significant bit to
+   indicate whether a function is virtual, ensure a pointer
+   to this function will have that bit clear.  */
+#define MINIMUM_METHOD_BOUNDARY \
+  ((TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)	     \
+   ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)
+
 
 
 extern void push_function_context (void);
Index: java/class.c
===================================================================
--- java/class.c	(revision 228756)
+++ java/class.c	(working copy)
@@ -779,13 +779,8 @@  add_method_1 (tree this_class, int acces
   DECL_CHAIN (fndecl) = TYPE_METHODS (this_class);
   TYPE_METHODS (this_class) = fndecl;
 
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && !(access_flags & ACC_STATIC)
-      && DECL_ALIGN (fndecl) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fndecl) = 2 * BITS_PER_UNIT;
+  if (!(access_flags & ACC_STATIC))
+    DECL_ALIGN (fndecl) = MINIMUM_METHOD_BOUNDARY;
 
   /* Notice that this is a finalizer and update the class type
      accordingly. This is used to optimize instance allocation. */