Re: [PATCH v7] forwprop: add simplify_phi_result_movdiv() [PR101179]

On 8/13/2026 12:06 PM, Jeffrey Law wrote:
> 
> 
> On 8/12/2026 2:42 PM, Daniel Barboza wrote:
>> This new forwprop  step is my attempt to implement Richi's suggestions
>> from v1 of this work [1] where he suggested to push things out of
>> match.pd.
>>
>> The idea is to simplify DIV/MOD into RSHIFT/BIT_AND ops in which the
>> divisor are pow2 integers in a PHI.  E.g.:
>>
>> - for MOD, if either "_y" is known positive or "_x" is used just in zero
>>    comparisons:
>>
>> phi_var = PHI <16,4>
>> _x = _y % phi_var
>>
>> Can be turned into:
>>
>> phi_var = PHI <15,3>
>> _x = _y & phi_var
>>
>> - for DIV, if "_y" is a known positive:
>>
>> phi_var = PHI <16,4>
>> _x = _y / phi_var
>>
>> Can be turned into:
>>
>> phi_var = PHI <4,2>
>> _x = _y >> phi_var
>>
>> Most of 101179 use cases are solved by this change.  PHI with 2+ args
>> are supported as long as every phi_arg meets the criteria.
>>
>> Boostrapped and regression tested with x86_64, aarch64 and riscv64.
>>
>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2026-May/716303.html
>>
>>     PR tree-optimization/101179
>>
>> gcc/ChangeLog:
>>
>>     * tree-ssa-forwprop.cc (simplify_phi_result_movdiv): New
>>     forwprop step where MOD/DIV ops with pow2 divisors can be
>>     simplified to BIT_AND/RSHIFT.
>>     (pass_forwprop::execute): Call simplify_phi_result_movdiv.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     * gcc.dg/tree-ssa/pr101179.c: New test.
> So I hate to have to bring up more issues, but I think this is broken for CEIL and ROUND variants.
> 
> Let's consider  3/4 with CEIL_DIV_EXPR, the result of that should be 1. But I think with this patch we get 0.  Similarly for ROUND_DIV_EXPR and the MOD variants I believe.

Hm, true.  The transformation was inspired in how div/mod works in C/C++
and I failed to do a proper check for the other variations :(


So for TRUNC_MOD/TRUNC_DIV we're good.  Since we're always dealing with
positive pow2 divisors I believe FLOOR_MOD/FLOOR_DIV is also ok.  And
we could also support EXACT_DIV too.

> 
> 
>> +
>> +  tree type = TREE_TYPE (gimple_phi_result (phi));
>> +  tree new_phires = make_ssa_name (type);
>> +  gphi *new_phi = create_phi_node (new_phires, phi->bb);
>> +
>> +  for (unsigned int i = 0; i < gimple_phi_num_args (phi); i++)
>> +    {
>> +      tree phi_arg = gimple_phi_arg_def (phi, i);
>> +      tree arg;
>> +
>> +      if (new_code == RSHIFT_EXPR)
>> +    arg = wide_int_to_tree (type, wi::exact_log2 (wi::to_wide (phi_arg)));
>> +      else
>> +    arg = wide_int_to_tree (type, wi::to_wide (phi_arg) - 1);
>> +
>> +      SET_PHI_ARG_DEF (new_phi, i, arg);
>> +    }
> Do you want to also copy any location information from the old phi to new phi?  See gimple_phi_arg_location and friends for the API to get and set the locations.  You might also want to look at add_phi_arg, it may (or may not) be a better API to work with.

I'll use add_phi_arg() and set the phi location accordingly.

Thanks,
Daniel
> 
> 
> 
>> +
>> +  /* Add a gimple_convert to integer_type_node for new_phires
>> +     since it might be a long long which we want to convert
>> +     into an integer or a bit_int that we want to convert into
>> +     an integer.  */
>> +  gimple_stmt_iterator gsi;
>> +  if (new_code == RSHIFT_EXPR)
>> +    {
>> +      gsi = gsi_for_stmt (stmt);
>> +      new_phires = gimple_convert (&gsi, true, GSI_SAME_STMT,
>> +                   gimple_location (stmt),
>> +                   integer_type_node, new_phires);
>> +    }
> Is integer_type_node the right type here?  Shouldn't we be getting the type from somewhere in the IL rather than assuming integer_type_node is correct?
> 
> jeff