Re: [PATCH, fortran, PR 82869] Introduce default_logical_type_node and logical_type_node

[Steve Kargl <sgk@troutmask.apl.washington.edu>]

On Tue, Nov 07, 2017 at 08:26:15PM +0200, Janne Blomqvist wrote:
> Earlier GFortran used to redefine boolean_type_node, which in the rest
> of the compiler means the C/C++ _Bool/bool type, to the Fortran
> default logical type.  When this redefinition was removed, a few
> issues surfaced. Namely,
> 
> 1) PR 82869, where we created a boolean tmp variable, and passed it to
> the runtime library as a Fortran logical variable of a different size.
> 
> 2) Fortran specifies that logical operations should be done with the
> default logical kind, not in any other kind.
> 
> 3) Using 8-bit variables have some issues, such as
>    - on x86, partial register stalls and length prefix changes.
>    - s390 has a compare with immediate and jump instruction which
>      works with 32-bit but not 8-bit quantities.
> 
> This patch addresses (2) by introducing a type
> default_logical_type_node which is used when evaluating Fortran
> logical expressions. (3) is addressed by introducing
> logical_type_node, a tree representing a logical(kind=4) type which
> can be used for compiler-generated temporary
> variables. logical_type_node is always 4 bytes. As a side effect, (1)
> is also fixed, though there might be some latent bug lurking there
> still.
> 
> For x86-64, using the Polyhedron benchmark suite, no performance or
> code size difference worth mentioning was observed.
> 
> Regtested on x86_64-pc-linux-gnu. Ok for trunk?

I scanned the patch and it looks ok to me (most a
mechanical find/replace operation).  One thing those,

> diff --git a/gcc/fortran/trans-types.c b/gcc/fortran/trans-types.c
> index 78477a9..602d369 100644
> --- a/gcc/fortran/trans-types.c
> +++ b/gcc/fortran/trans-types.c
> @@ -62,6 +62,12 @@ tree ppvoid_type_node;
>  tree pchar_type_node;
>  tree pfunc_type_node;
>  
> +tree default_logical_type_node;
> +tree default_logical_true_node;
> +tree default_logical_false_node;
> +tree logical_type_node;
> +tree logical_true_node;
> +tree logical_false_node;
>  tree gfc_charlen_type_node;
>  
>  tree gfc_float128_type_node = NULL_TREE;
> @@ -1003,6 +1009,15 @@ gfc_init_types (void)
>  			wi::mask (n, UNSIGNED,
>  				  TYPE_PRECISION (size_type_node)));
>  
> +
> +  default_logical_type_node = gfc_get_logical_type (gfc_default_logical_kind);
> +  default_logical_true_node = build_int_cst (default_logical_type_node, 1);
> +  default_logical_false_node = build_int_cst (default_logical_type_node, 0);
> +
> +  logical_type_node = gfc_get_logical_type (4);

Can you add a comment to note that the 4 is purposely chosen?
A year or so from now, someone might change this to gfc_default_logical_kind
without understand/recalling why 4 is used.

-- 
steve