Hi Andi, This seems like a reasonable way to avoid the specific issue in PR117091 and generally speed up switch lowering of switches with all cases unique. I cannot approve this but want to share some comments.
On Wed 2024-10-16 17:50:59, Andi Kleen wrote: > diff --git a/gcc/gimple-if-to-switch.cc b/gcc/gimple-if-to-switch.cc > index 96ce1c380a59..4151d1bb520e 100644 > --- a/gcc/gimple-if-to-switch.cc > +++ b/gcc/gimple-if-to-switch.cc > @@ -254,7 +254,7 @@ if_chain::is_beneficial () > else > output.release (); > > - output = bit_test_cluster::find_bit_tests (filtered_clusters); > + output = bit_test_cluster::find_bit_tests (filtered_clusters, 2); Maybe it would be nicer to not have max_c as a parameter of find_bit_tests() but just guard the call to find_bit_tests() in analyze_switch_statement() with max_c > 2. Since we don't have max_c in if_chain::is_beneficial(), having max_c as parameter leads to this constant 2 in this call that will possibly confuse people reading if_chain::is_beneficial(). But this is a minor thing and I guess your way (max_c as a parameter) is also fine by me. > r = output.length () < filtered_clusters.length (); > if (r) > dump_clusters (&output, "BT can be built"); > diff --git a/gcc/tree-switch-conversion.cc b/gcc/tree-switch-conversion.cc > index 00426d400006..bb7b8cf215a3 100644 > --- a/gcc/tree-switch-conversion.cc > +++ b/gcc/tree-switch-conversion.cc > @@ -1772,12 +1772,13 @@ jump_table_cluster::is_beneficial (const vec<cluster > *> &, > } > > /* Find bit tests of given CLUSTERS, where all members of the vector > - are of type simple_cluster. New clusters are returned. */ > + are of type simple_cluster. max_c is the max number of cases per label. There should be two spaces before "max_c". Also it would be nice if MAX_C was written in uppercase for consistency with other function comments in tree-switch-conversion.cc. > @@ -577,8 +577,9 @@ public: > bool try_switch_expansion (vec<cluster *> &clusters); > /* Compute the number of case labels that correspond to each outgoing edge > of > switch statement. Record this information in the aux field of the edge. > + Returns max number of cases per edge. > */ I would specify "approx max number" instead of "max number" here. Otherwise looks good to me. Cheers, Filip Kastl
