[PATCH] Fix recent thinko in operand_equal_p

[PATCH] Fix recent thinko in operand_equal_p

[Eric Botcazou]

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

Hi,

there is a thinko in the recent improvement by Jan:

2020-11-19  Jan Hubicka  <jh@suse.cz>

	* fold-const.c (operand_compare::operand_equal_p): Fix thinko in
	COMPONENT_REF handling and guard types_same_for_odr by
	virtual_method_call_p.
	(operand_compare::hash_operand): Likewise.

where the code just looks at operand 2 of COMPONENT_REF, if it is present, to 
compare addresses.  That's wrong because operand 2 contains the number of 
DECL_OFFSET_ALIGN-bit-sized words so, when DECL_OFFSET_ALIGN > 8, not all the 
bytes are included and some of them are in DECL_FIELD_BIT_OFFSET instead, see 
get_inner_reference for the model computation.

In other words, you would need to compare operand 2 and DECL_OFFSET_ALIGN and 
DECL_FIELD_BIT_OFFSET in this situation, but I'm not sure this is worth the 
hassle in practice so the attached fix just removes this alternate handling.

Tested on x86-64/Linux, OK for mainline, 12 and 11 branches?


2022-11-04  Eric Botcazou  <ebotcazou@adacore.com>

	* fold-const.cc (operand_compare::operand_equal_p) <COMPONENT_REF>:
	Do not take into account operand 2.
	(operand_compare::hash_operand) <COMPONENT_REF>: Likewise.


2022-11-04  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/opt99.adb: New test.
	* gnat.dg/opt99_pkg1.ads, gnat.dg/opt99_pkg1.adb: New helper.
	* gnat.dg/opt99_pkg2.ads: Likewise.

-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1304 bytes --]

diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index 9f7beae14e5..e7a9d5215a5 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -3351,9 +3351,6 @@ operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
 		if (compare_address
 		    && (flags & OEP_ADDRESS_OF_SAME_FIELD) == 0)
 		  {
-		    if (TREE_OPERAND (arg0, 2)
-			|| TREE_OPERAND (arg1, 2))
-		      return OP_SAME_WITH_NULL (2);
 		    tree field0 = TREE_OPERAND (arg0, 1);
 		    tree field1 = TREE_OPERAND (arg1, 1);
 
@@ -3864,17 +3861,10 @@ operand_compare::hash_operand (const_tree t, inchash::hash &hstate,
 	      if (sflags & OEP_ADDRESS_OF)
 		{
 		  hash_operand (TREE_OPERAND (t, 0), hstate, flags);
-		  if (TREE_OPERAND (t, 2))
-		    hash_operand (TREE_OPERAND (t, 2), hstate,
-				  flags & ~OEP_ADDRESS_OF);
-		  else
-		    {
-		      tree field = TREE_OPERAND (t, 1);
-		      hash_operand (DECL_FIELD_OFFSET (field),
-				    hstate, flags & ~OEP_ADDRESS_OF);
-		      hash_operand (DECL_FIELD_BIT_OFFSET (field),
-				    hstate, flags & ~OEP_ADDRESS_OF);
-		    }
+		  hash_operand (DECL_FIELD_OFFSET (TREE_OPERAND (t, 1)),
+				hstate, flags & ~OEP_ADDRESS_OF);
+		  hash_operand (DECL_FIELD_BIT_OFFSET (TREE_OPERAND (t, 1)),
+				hstate, flags & ~OEP_ADDRESS_OF);
 		  return;
 		}
 	      break;

[-- Attachment #3: opt99.adb --]
[-- Type: text/x-adasrc, Size: 248 bytes --]

-- { dg-do run }
-- { dg-options "-O" }

with Opt99_Pkg1; use Opt99_Pkg1;

procedure Opt99 is
  C : constant My_Character := (D => True, C => ' ');
  D : Derived;

begin
  Set (D, C, C);
  if not D.C2.D then
    raise Program_Error;
  end if;
end;

[-- Attachment #4: opt99_pkg1.adb --]
[-- Type: text/x-adasrc, Size: 169 bytes --]

package body Opt99_Pkg1 is

  procedure Set (D: in out Derived; C1, C2 : My_Character) is
  begin
    D.I  := 0;
    D.C1 := C1;
    D.C2 := C2;
  end;

end Opt99_Pkg1;

[-- Attachment #5: opt99_pkg2.ads --]
[-- Type: text/x-adasrc, Size: 222 bytes --]

package Opt99_Pkg2 is

  function Get_Max return Positive is (4);

  C : constant Positive := Get_Max;

  type Arr is array (1 .. C) of Integer;

  type Root is tagged record
    Data : Arr;
  end record;

end Opt99_Pkg2;

[-- Attachment #6: opt99_pkg1.ads --]
[-- Type: text/x-adasrc, Size: 384 bytes --]

with Opt99_Pkg2;

package Opt99_Pkg1 is

  type My_Character (D : Boolean := False) is record
    case D is
      when False => null;
      when True  => C : Character;
    end case;
  end record;

  type Derived is new Opt99_Pkg2.Root with record
    I : Integer;
    C1, C2 : My_Character;
  end record;

  procedure Set (D: in out Derived; C1, C2 : My_Character);

end Opt99_Pkg1;

Re: [PATCH] Fix recent thinko in operand_equal_p

[Richard Biener]

> Am 04.11.2022 um 10:29 schrieb Eric Botcazou via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi,
> 
> there is a thinko in the recent improvement by Jan:
> 
> 2020-11-19  Jan Hubicka  <jh@suse.cz>
> 
>    * fold-const.c (operand_compare::operand_equal_p): Fix thinko in
>    COMPONENT_REF handling and guard types_same_for_odr by
>    virtual_method_call_p.
>    (operand_compare::hash_operand): Likewise.
> 
> where the code just looks at operand 2 of COMPONENT_REF, if it is present, to 
> compare addresses.  That's wrong because operand 2 contains the number of 
> DECL_OFFSET_ALIGN-bit-sized words so, when DECL_OFFSET_ALIGN > 8, not all the 
> bytes are included and some of them are in DECL_FIELD_BIT_OFFSET instead, see 
> get_inner_reference for the model computation.
> 
> In other words, you would need to compare operand 2 and DECL_OFFSET_ALIGN and 
> DECL_FIELD_BIT_OFFSET in this situation, but I'm not sure this is worth the 
> hassle in practice so the attached fix just removes this alternate handling.

> Tested on x86-64/Linux, OK for mainline, 12 and 11 branches?

Ok.

Thanks,
Richard 

> 
> 2022-11-04  Eric Botcazou  <ebotcazou@adacore.com>
> 
>    * fold-const.cc (operand_compare::operand_equal_p) <COMPONENT_REF>:
>    Do not take into account operand 2.
>    (operand_compare::hash_operand) <COMPONENT_REF>: Likewise.
> 
> 
> 2022-11-04  Eric Botcazou  <ebotcazou@adacore.com>
> 
>    * gnat.dg/opt99.adb: New test.
>    * gnat.dg/opt99_pkg1.ads, gnat.dg/opt99_pkg1.adb: New helper.
>    * gnat.dg/opt99_pkg2.ads: Likewise.
> 
> -- 
> Eric Botcazou
> <p.diff>
> <opt99.adb>
> <opt99_pkg1.adb>
> <opt99_pkg2.ads>
> <opt99_pkg1.ads>