| Message ID | 20201119123353.GB46107@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix hanlding of gimple_clobber in ICF | expand |
On Thu, 19 Nov 2020, Jan Hubicka wrote: > Hi, > after fixing few issues I gotto stage where 1.4M icf mismatches are due to > comparing two gimple clobber. The problem is that operand_equal_p match > clobber > > case CONSTRUCTOR: > /* In GIMPLE empty constructors are allowed in initializers of > aggregates. */ > return !CONSTRUCTOR_NELTS (arg0) && !CONSTRUCTOR_NELTS (arg1); > > But this happens too late after comparing semantics of its type (that > are not very relevant for memory store and fails way too often). > > In the context of ipa-icf we do not really need to match RHS of gimple clobbers: > it is enough to know that the LHS stores can be considered equivalent. > > I this added logic to hash them all the same way and compare using > TREE_CLOBBER_P flag. I see other option in extending operand_equal_p > in fold-const to handle them more generously or making stmt hash and compare > to skip comparing/hashing RHS of gimple_clobber_p. > > I am now down to 1453 opernad_equal_p mismatches so it seems we are getting > into shape. > > Bootstrapped/regtested x86_64, looks reasonable? > Honza > > * ipa-icf-gimple.c (func_checker::hash_operand): Hash gimple clobber. > (func_checker::operand_equal_p): Special case gimple clobber. > diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c > index ffb1baddbdb..69bc9ab5b34 100644 > --- a/gcc/ipa-icf-gimple.c > +++ b/gcc/ipa-icf-gimple.c > @@ -245,6 +245,14 @@ func_checker::hash_operand (const_tree arg, inchash::hash &hstate, > break; > } > > + /* In gimple all clobbers can be considered equal: while comparaing two > + gimple clobbers we match the left hand memory accesses. */ > + if (TREE_CLOBBER_P (arg)) > + { > + hstate.add_int (0xc10bbe5); > + return; > + } > + > return operand_compare::hash_operand (arg, hstate, flags); > } > > @@ -306,6 +314,10 @@ func_checker::operand_equal_p (const_tree t1, const_tree t2, > default: > break; > } > + /* In gimple all clobbers can be considered equal. We match the right hand left hand otherwise yes, I guess this will work for ICF. > + memory accesses. */ > + if (TREE_CLOBBER_P (t1) || TREE_CLOBBER_P (t2)) > + return TREE_CLOBBER_P (t1) == TREE_CLOBBER_P (t2); > > return operand_compare::operand_equal_p (t1, t2, flags); > } >
Hi Honza! On 2020-11-19T13:33:53+0100, Jan Hubicka <hubicka@ucw.cz> wrote: > --- a/gcc/ipa-icf-gimple.c > +++ b/gcc/ipa-icf-gimple.c > @@ -245,6 +245,14 @@ func_checker::hash_operand (const_tree arg, inchash::hash &hstate, > break; > } > > + /* In gimple all clobbers can be considered equal: while comparaing two > + gimple clobbers we match the left hand memory accesses. */ > + if (TREE_CLOBBER_P (arg)) > + { > + hstate.add_int (0xc10bbe5); > + return; > + } > + > return operand_compare::hash_operand (arg, hstate, flags); > } I understand correctly that 0xc10bbe5 here just a "random"/fixed number (maybe should be documented as such?), so that we get stable behavior for any 'TREE_CLOBBER_P (arg)' cases, and it isn't meant to convey any meaning (that I'm not seeing)? Grüße Thomas > @@ -306,6 +314,10 @@ func_checker::operand_equal_p (const_tree t1, const_tree t2, > default: > break; > } > + /* In gimple all clobbers can be considered equal. We match the right hand > + memory accesses. */ > + if (TREE_CLOBBER_P (t1) || TREE_CLOBBER_P (t2)) > + return TREE_CLOBBER_P (t1) == TREE_CLOBBER_P (t2); > > return operand_compare::operand_equal_p (t1, t2, flags); > } ----------------- Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
> Hi Honza! > > On 2020-11-19T13:33:53+0100, Jan Hubicka <hubicka@ucw.cz> wrote: > > --- a/gcc/ipa-icf-gimple.c > > +++ b/gcc/ipa-icf-gimple.c > > @@ -245,6 +245,14 @@ func_checker::hash_operand (const_tree arg, inchash::hash &hstate, > > break; > > } > > > > + /* In gimple all clobbers can be considered equal: while comparaing two > > + gimple clobbers we match the left hand memory accesses. */ > > + if (TREE_CLOBBER_P (arg)) > > + { > > + hstate.add_int (0xc10bbe5); > > + return; > > + } > > + > > return operand_compare::hash_operand (arg, hstate, flags); > > } > > I understand correctly that 0xc10bbe5 here just a "random"/fixed number > (maybe should be documented as such?), so that we get stable behavior for > any 'TREE_CLOBBER_P (arg)' cases, and it isn't meant to convey any > meaning (that I'm not seeing)? Yes, we need to avoid calling hash_operand becaue we do not want to include additional info it hashes and we want to have good chance not colliding with other items operand hashing adds. It usually (apparently not always) adds first gimple code, so hashing it random large number is quite safe. Honza > > > Grüße > Thomas > > > > @@ -306,6 +314,10 @@ func_checker::operand_equal_p (const_tree t1, const_tree t2, > > default: > > break; > > } > > + /* In gimple all clobbers can be considered equal. We match the right hand > > + memory accesses. */ > > + if (TREE_CLOBBER_P (t1) || TREE_CLOBBER_P (t2)) > > + return TREE_CLOBBER_P (t1) == TREE_CLOBBER_P (t2); > > > > return operand_compare::operand_equal_p (t1, t2, flags); > > } > ----------------- > Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c index ffb1baddbdb..69bc9ab5b34 100644 --- a/gcc/ipa-icf-gimple.c +++ b/gcc/ipa-icf-gimple.c @@ -245,6 +245,14 @@ func_checker::hash_operand (const_tree arg, inchash::hash &hstate, break; } + /* In gimple all clobbers can be considered equal: while comparaing two + gimple clobbers we match the left hand memory accesses. */ + if (TREE_CLOBBER_P (arg)) + { + hstate.add_int (0xc10bbe5); + return; + } + return operand_compare::hash_operand (arg, hstate, flags); } @@ -306,6 +314,10 @@ func_checker::operand_equal_p (const_tree t1, const_tree t2, default: break; } + /* In gimple all clobbers can be considered equal. We match the right hand + memory accesses. */ + if (TREE_CLOBBER_P (t1) || TREE_CLOBBER_P (t2)) + return TREE_CLOBBER_P (t1) == TREE_CLOBBER_P (t2); return operand_compare::operand_equal_p (t1, t2, flags); }
Hi, after fixing few issues I gotto stage where 1.4M icf mismatches are due to comparing two gimple clobber. The problem is that operand_equal_p match clobber case CONSTRUCTOR: /* In GIMPLE empty constructors are allowed in initializers of aggregates. */ return !CONSTRUCTOR_NELTS (arg0) && !CONSTRUCTOR_NELTS (arg1); But this happens too late after comparing semantics of its type (that are not very relevant for memory store and fails way too often). In the context of ipa-icf we do not really need to match RHS of gimple clobbers: it is enough to know that the LHS stores can be considered equivalent. I this added logic to hash them all the same way and compare using TREE_CLOBBER_P flag. I see other option in extending operand_equal_p in fold-const to handle them more generously or making stmt hash and compare to skip comparing/hashing RHS of gimple_clobber_p. I am now down to 1453 opernad_equal_p mismatches so it seems we are getting into shape. Bootstrapped/regtested x86_64, looks reasonable? Honza * ipa-icf-gimple.c (func_checker::hash_operand): Hash gimple clobber. (func_checker::operand_equal_p): Special case gimple clobber.