> > The `IFN_CLZ/IFN_CTZ` pattern seems too permissive I will change the check to `>= precision`.
> > This adds match.pd simplifications for min(clz(x), clz(y)) and > > min(ctz(x), ctz(y)) to clz(x | y) and ctz(x | y). > > > > PR tree-optimization/123311 > > > > gcc/ChangeLog: > > > > * match.pd (min(clz(x), clz(y)) -> clz(x | y)): New pattern. > > (min(ctz(x), ctz(y)) -> ctz(x | y)): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/tree-ssa/pr123311.c: New test. > Don't we have to test the CLZ_DEFINED_VALUE_AT_ZERO and > CTZ_DEFINED_VALUE_AT_ZERO? > __builtin_ctz(0)/clz already has undefined behaviour and IFN_CTZ/IFN_CLZ defines the value at zero. Is the above test still needed? > +/* min (clz (x), clz (y)) -> clz (x | y) and > > + min (ctz (x), ctz (y)) -> ctz (x | y). */ > > +(for func (CLZ CTZ) > > + (simplify > > + (min (func:s @0) (func:s @1)) > > + (if (types_match (@0, @1) > > + && (!sanitize_flags_p (SANITIZE_BUILTIN) > > + || (cfun && (cfun->curr_properties & PROP_ssa) != 0))) > Hmm. Can you explain what these two trailing conditions are doing as a > comment in this patch? > > I'm guessing you want to give the sanitizers a chance to instrument the > builtin? But better to be sure. > The patch https://gcc.gnu.org/cgit/gcc/commit?id=f7d1b9cd added it to other ctz/clz patterns. I did the same for the new pattern. This is added for UBSan to instrument similar to other CTZ/CLZ patterns in match.pd. IFN_CTZ does not have this check in other patterns. Regards, Eikansh On Mon, Jun 29, 2026 at 11:18 PM [email protected] < [email protected]> wrote: > Mostly looks reasonable, but I see a couple things worth calling out. > > ### Main correctness concern > The `IFN_CLZ/IFN_CTZ` pattern seems too permissive: > > ```c > (if (types_match (@0, @1) > && wi::geu_p (wi::to_wide (@2), > TYPE_PRECISION (TREE_TYPE (@0)) - 1)) > (func (bit_ior @0 @1) @2)) > ``` > > For the 2-arg generic forms, the second argument is the value returned > for zero. > The identity > > - `min(ctzg(x, z), ctzg(y, z)) == ctzg(x|y, z)` > - `min(clzg(x, z), clzg(y, z)) == clzg(x|y, z)` > > only holds when `z` is at least the **full precision**, not `precision - > 1`. > > Counterexample for `ctzg` with 32-bit type and `z = 31`: > - `x = 0`, `y = 1` > - `ctzg(0,31) = 31` > - `ctzg(1,31) = 0` > - `min(...) = 0` > - `x|y = 1`, so `ctzg(1,31) = 0` — fine here > > But for `clzg`: > - `x = 0`, `y = 0x80000000` > - `clzg(0,31) = 31` > - `clzg(0x80000000,31) = 0` > - `min(...) = 0` > - `x|y = 0x80000000`, result `0` — also fine > > The real problematic case is both zero: > - `min(func(0,z), func(0,z)) = z` > - `func(0|0,z) = z` > > So the above examples don't fail, but the guard still looks suspicious. > The mathematically natural bound is `z >= precision`, because ordinary > `clz/ctz` on nonzero values range from `0..precision-1`, and you want > zero's fallback not to become smaller than any nonzero result. With `z = > precision-1`, zero becomes indistinguishable from the maximal nonzero > result, which may still be OK for `min`, but I'd want this justified > explicitly. As written, this is subtle enough that it deserves either a > proof in the comment or tightening to `>= precision` if that was the > actual intent. > > > > > On 6/28/2026 11:26 PM, Eikansh Gupta wrote: > > This adds match.pd simplifications for min(clz(x), clz(y)) and > > min(ctz(x), ctz(y)) to clz(x | y) and ctz(x | y). > > > > PR tree-optimization/123311 > > > > gcc/ChangeLog: > > > > * match.pd (min(clz(x), clz(y)) -> clz(x | y)): New pattern. > > (min(ctz(x), ctz(y)) -> ctz(x | y)): Likewise. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/tree-ssa/pr123311.c: New test. > Don't we have to test the CLZ_DEFINED_VALUE_AT_ZERO and > CTZ_DEFINED_VALUE_AT_ZERO? > > > > > > +/* min (clz (x), clz (y)) -> clz (x | y) and > > + min (ctz (x), ctz (y)) -> ctz (x | y). */ > > +(for func (CLZ CTZ) > > + (simplify > > + (min (func:s @0) (func:s @1)) > > + (if (types_match (@0, @1) > > + && (!sanitize_flags_p (SANITIZE_BUILTIN) > > + || (cfun && (cfun->curr_properties & PROP_ssa) != 0))) > Hmm. Can you explain what these two trailing conditions are doing as a > comment in this patch? > > I'm guessing you want to give the sanitizers a chance to instrument the > builtin? But better to be sure. > > > > > > + (func (bit_ior @0 @1))))) > > +(for func (IFN_CLZ IFN_CTZ) > > + (simplify > > + (min (func:s @0 INTEGER_CST@2) (func:s @1 @2)) > > + (if (types_match (@0, @1) > > + && wi::geu_p (wi::to_wide (@2), > > + TYPE_PRECISION (TREE_TYPE (@0)) - 1)) > > + (func (bit_ior @0 @1) @2)))) > And why don't we need to do the same or this pattern? > > Mostly looks good, just those few nits to chase down. > > Jeff
