Fix gimplification of ordering comparisons of arrays of bytes

Message ID 4904981.GXAFRqVoOG@fomalhaut
State New
Headers
Series Fix gimplification of ordering comparisons of arrays of bytes |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Eric Botcazou July 8, 2024, 7:08 a.m. UTC
  Hi,

the Ada compiler now defers to the gimplifier for ordering comparisons of 
arrays of bytes (Ada parlance for <, >, <= and >=) because the gimplifier in 
turn defers to memcmp for them, which implements the required semantics.

However the gimplifier has a special processing for aggregate types whose mode 
is not BLKmode and this processing deviates from the memcmp semantics when the 
target is little-endian.

Tested on x86-64/Linux, OK for the mainline?


2024-07-08  Eric Botcazou  <ebotcazou@adacore.com>

	* gimplify.cc (gimplify_scalar_mode_aggregate_compare): Add support
	for ordering comparisons.


2024-07-08  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/array42.adb,/gnat.dg/array42_pkg.ads: New test.
  

Comments

Richard Biener July 8, 2024, 8:15 a.m. UTC | #1
On Mon, Jul 8, 2024 at 9:11 AM Eric Botcazou <botcazou@adacore.com> wrote:
>
> Hi,
>
> the Ada compiler now defers to the gimplifier for ordering comparisons of
> arrays of bytes (Ada parlance for <, >, <= and >=) because the gimplifier in
> turn defers to memcmp for them, which implements the required semantics.
>
> However the gimplifier has a special processing for aggregate types whose mode
> is not BLKmode and this processing deviates from the memcmp semantics when the
> target is little-endian.
>
> Tested on x86-64/Linux, OK for the mainline?

+  if (code != EQ_EXPR && code != NE_EXPR)
+    {
+      gcc_assert (BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN);
+      gcc_assert (TREE_CODE (scalar_type) == INTEGER_TYPE);

IIRC at least some array of vector also have vector mode, I'm not sure
type_for_mode is an INTEGER_TYPE in that case nor am I sure
the max 128 bytes size holds here.  There's also
array_mode_supported_p which can indicate larger modes are OK.

That said, when BYTES_BIG_ENDIAN the outcome is better
than before.

If we do not have any BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN
target then maybe it's time to retire WORDS_BIG_ENDIAN?  Otherwise
is there a fallback solution?  I wonder if we can fall back to
gimplify_variable_sized_compare if gimplify_scalar_mode_aggregate_compare
"fails".

Richard.

>
> 2024-07-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimplify.cc (gimplify_scalar_mode_aggregate_compare): Add support
>         for ordering comparisons.
>
>
> 2024-07-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/array42.adb,/gnat.dg/array42_pkg.ads: New test.
>
> --
> Eric Botcazou
  
Eric Botcazou July 8, 2024, 2:39 p.m. UTC | #2
> IIRC at least some array of vector also have vector mode, I'm not sure
> type_for_mode is an INTEGER_TYPE in that case nor am I sure the max 128
> bytes size holds here.

Yes, array types may have vector modes.

> There's also array_mode_supported_p which can indicate larger modes are OK.

Yes, but only for ARM and Aarch64 and they return true only if the element 
type already has vector mode.

> That said, when BYTES_BIG_ENDIAN the outcome is better han before.

The wrong code is actually for little-endian targets here, e.g. x86-64.

> If we do not have any BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN
> target then maybe it's time to retire WORDS_BIG_ENDIAN?

I indeed don't think that we still have this kind of targets.

> Otherwise is there a fallback solution?  I wonder if we can fall back to
> gimplify_variable_sized_compare if gimplify_scalar_mode_aggregate_compare
> "fails".

Falling back to gimplify_scalar_mode_aggregate_compare should always be OK if 
the mode is not integral scalar, as documented in the description:

/* Gimplify a comparison between two aggregate objects of integral scalar
   mode as a comparison between the bitwise equivalent scalar values.  */

static enum gimplify_status
gimplify_scalar_mode_aggregate_compare (tree *expr_p)

so we could change the guard to match the description exactly:

	  else if (SCALAR_INT_MODE_P (TYPE_MODE (type)))
	    ret = gimplify_scalar_mode_aggregate_compare (expr_p);
	  else
	    ret = gimplify_variable_sized_compare (expr_p);


The new code is only exercised for Ada I think (otherwise there is wrong code 
generated on little-endian targets in other languages as of this writing) and 
it works on all the platforms we test (a bunch) so any additional path would 
be uncovered.  If we have evidences that it's not sufficient, then I'm ready 
to amend it, but at the moment it would be a bit of a shot in the dark.
  
Richard Biener July 8, 2024, 2:55 p.m. UTC | #3
> Am 08.07.2024 um 16:39 schrieb Eric Botcazou <botcazou@adacore.com>:
> 
> 
>> 
>> IIRC at least some array of vector also have vector mode, I'm not sure
>> type_for_mode is an INTEGER_TYPE in that case nor am I sure the max 128
>> bytes size holds here.
> 
> Yes, array types may have vector modes.
> 
>> There's also array_mode_supported_p which can indicate larger modes are OK.
> 
> Yes, but only for ARM and Aarch64 and they return true only if the element
> type already has vector mode.
> 
>> That said, when BYTES_BIG_ENDIAN the outcome is better han before.
> 
> The wrong code is actually for little-endian targets here, e.g. x86-64.
> 
>> If we do not have any BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN
>> target then maybe it's time to retire WORDS_BIG_ENDIAN?
> 
> I indeed don't think that we still have this kind of targets.
> 
>> Otherwise is there a fallback solution?  I wonder if we can fall back to
>> gimplify_variable_sized_compare if gimplify_scalar_mode_aggregate_compare
>> "fails".
> 
> Falling back to gimplify_scalar_mode_aggregate_compare should always be OK if
> the mode is not integral scalar, as documented in the description:
> 
> /* Gimplify a comparison between two aggregate objects of integral scalar
>   mode as a comparison between the bitwise equivalent scalar values.  */
> 
> static enum gimplify_status
> gimplify_scalar_mode_aggregate_compare (tree *expr_p)
> 
> so we could change the guard to match the description exactly:
> 
>      else if (SCALAR_INT_MODE_P (TYPE_MODE (type)))

Yeah, I prefer this to the != BLKmode check.

So OK with this change.

Richard 

>        ret = gimplify_scalar_mode_aggregate_compare (expr_p);
>      else
>        ret = gimplify_variable_sized_compare (expr_p);
> 
> 
> The new code is only exercised for Ada I think (otherwise there is wrong code
> generated on little-endian targets in other languages as of this writing) and
> it works on all the platforms we test (a bunch) so any additional path would
> be uncovered.  If we have evidences that it's not sufficient, then I'm ready
> to amend it, but at the moment it would be a bit of a shot in the dark.
> 
> --
> Eric Botcazou
> 
>
  

Patch

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 5a9627c4acf..c81900af970 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -6728,18 +6728,56 @@  gimplify_variable_sized_compare (tree *expr_p)
 static enum gimplify_status
 gimplify_scalar_mode_aggregate_compare (tree *expr_p)
 {
-  location_t loc = EXPR_LOCATION (*expr_p);
+  const location_t loc = EXPR_LOCATION (*expr_p);
+  const enum tree_code code = TREE_CODE (*expr_p);
   tree op0 = TREE_OPERAND (*expr_p, 0);
   tree op1 = TREE_OPERAND (*expr_p, 1);
-
   tree type = TREE_TYPE (op0);
   tree scalar_type = lang_hooks.types.type_for_mode (TYPE_MODE (type), 1);
 
   op0 = fold_build1_loc (loc, VIEW_CONVERT_EXPR, scalar_type, op0);
   op1 = fold_build1_loc (loc, VIEW_CONVERT_EXPR, scalar_type, op1);
 
-  *expr_p
-    = fold_build2_loc (loc, TREE_CODE (*expr_p), TREE_TYPE (*expr_p), op0, op1);
+  /* We need to perform ordering comparisons in memory order like memcmp and,
+     therefore, may need to byte-swap operands for little-endian targets.  */
+  if (code != EQ_EXPR && code != NE_EXPR)
+    {
+      gcc_assert (BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN);
+      gcc_assert (TREE_CODE (scalar_type) == INTEGER_TYPE);
+      tree fndecl;
+
+      if (BYTES_BIG_ENDIAN)
+	fndecl = NULL_TREE;
+      else
+	switch (int_size_in_bytes (scalar_type))
+	  {
+	  case 1:
+	    fndecl = NULL_TREE;
+	    break;
+	  case 2:
+	    fndecl = builtin_decl_implicit (BUILT_IN_BSWAP16);
+	    break;
+	  case 4:
+	    fndecl = builtin_decl_implicit (BUILT_IN_BSWAP32);
+	    break;
+	  case 8:
+	    fndecl = builtin_decl_implicit (BUILT_IN_BSWAP64);
+	    break;
+	  case 16:
+	    fndecl = builtin_decl_implicit (BUILT_IN_BSWAP128);
+	    break;
+	  default:
+	    gcc_unreachable ();
+	  }
+
+      if (fndecl)
+	{
+	  op0 = build_call_expr_loc (loc, fndecl, 1, op0);
+	  op1 = build_call_expr_loc (loc, fndecl, 1, op1);
+	}
+    }
+
+  *expr_p = fold_build2_loc (loc, code, TREE_TYPE (*expr_p), op0, op1);
 
   return GS_OK;
 }