On Tue, Sep 5, 2023 at 12:09 AM Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 9/1/23 20:32, Andrew Pinski via Gcc-patches wrote: > > This turns out to be a latent bug in ssa_name_has_boolean_range > > where it would return true for all boolean types but all of the > > uses of ssa_name_has_boolean_range was expecting 0/1 as the range > > rather than [-1,0]. > > So when I fixed vector lower to do all comparisons in boolean_type > > rather than still in the signed-boolean:31 type (to fix a different issue), > > the pattern in match for `-(type)!A -> (type)A - 1.` would assume A (which > > was signed-boolean:31) had a range of [0,1] which broke down and sometimes > > gave us -1/-2 as values rather than what we were expecting of -1/0. > > > > This was the simpliest patch I found while testing. > > > > We have another way of matching [0,1] range which we could use instead > > of ssa_name_has_boolean_range except that uses only the global ranges > > rather than the local range (during VRP). > > I tried to clean this up slightly by using gimple_match_zero_one_valuedp > > inside ssa_name_has_boolean_range but that failed because due to using > > only the global ranges. I then tried to change get_nonzero_bits to use > > the local ranges at the optimization time but that failed also because > > we would remove branches to __builtin_unreachable during evrp and lose > > information as we don't set the global ranges during evrp. > > > > OK? Bootstrapped and tested on x86_64-linux-gnu. > > > > PR 110817 > > > > gcc/ChangeLog: > > > > * tree-ssanames.cc (ssa_name_has_boolean_range): Remove the > > check for boolean type as they don't have "[0,1]" range. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.c-torture/execute/pr110817-1.c: New test. > > * gcc.c-torture/execute/pr110817-2.c: New test. > > * gcc.c-torture/execute/pr110817-3.c: New test. > I'm a bit surprised this didn't trigger any regressions. Though maybe > all the existing testcases were capturing cases where non-boolean types > were known to have a 0/1 value.
Well except ssa_name_has_boolean_range will return true for `An [unsigned] integral type with a single bit of precision` which the normal boolean type for C is. So the only case where this makes a difference is signed booleans. Vectors and Ada are the only 2 places I know of which use signed booleans even. This came up before too; https://inbox.sourceware.org/gcc-patches/cafiyyc23zmevy6i9g1wpmpp7purcuzatg1qpwf2d_8n6f22...@mail.gmail.com/ . Anyways the 3 uses of ssa_name_has_boolean_range in match.pd are: /* X / bool_range_Y is X. */ which is not true for signed booleans; though division for boolean types is not well defined /* 1 - a is a ^ 1 if a had a bool range. */ Which is broken for signed booleans; though it might not show up in IR for non 1-bit boolean types. /* -(type)!A -> (type)A - 1. */ This one 100 % requires `A` and `A == 0` to be [0,1] range. The other uses of ssa_name_has_boolean_range are in DOM. The first 2 uses of ssa_name_has_boolean_range use build_one_cst/build_one_cst which is definitely wrong there. should have been constant_boolean_node for N-bit signed boolean types. The use `A COND_EXPR may create equivalences too.` actually does the correct thing and uses constant_boolean_node. Now maybe we miss some optimizations with Ada code with this change; I am not 100% sure. Maybe the change should just add && TYPE_UNSIGNED (type) to the check of boolean type and that will fix the issue too. > > > OK. > jeff
