On Thu, Sep 5, 2024 at 3:41 PM Jeff Law <j...@ventanamicro.com> wrote: > > > > On 9/5/24 12:38 PM, Raphael Zinsly wrote: > > On Thu, Sep 5, 2024 at 3:10 PM Jeff Law <jeffreya...@gmail.com> wrote: > >> On 9/5/24 6:16 AM, Raphael Zinsly wrote: > >>> On Wed, Sep 4, 2024 at 8:32 PM Jeff Law <j...@ventanamicro.com> wrote: > >>>> On 9/2/24 2:01 PM, Raphael Moreira Zinsly wrote: > >>>> ... > >>>>> + bool bit31 = (hival & 0x80000000) != 0; > >>>>> + int trailing_shift = ctz_hwi (loval) - ctz_hwi (hival); > >>>>> + int leading_shift = clz_hwi (loval) - clz_hwi (hival); > >>>>> + int shiftval = 0; > >>>>> + > >>>>> + /* Adjust the shift into the high half accordingly. */ > >>>>> + if ((trailing_shift > 0 && hival == (loval >> trailing_shift)) > >>>>> + || (trailing_shift < 0 && hival == (loval << trailing_shift))) > >>>>> + shiftval = 32 - trailing_shift; > >>>>> + else if ((leading_shift < 0 && hival == (loval >> leading_shift)) > >>>>> + || (leading_shift > 0 && hival == (loval << > >>>>> leading_shift))) > >>>> Don't these trigger undefined behavior when tailing_shift or > >>>> leading_shift is < 0? We shouldn't ever generate negative shift counts. > >>> > >>> The value of trailing/leading_shift is added to 32, we will never have > >>> negative shift counts. > >> In the IF you have this conditional: > >> > >>> (trailing_shift < 0 && hival == (loval << trailing_shift)) > >> > >> How could that not be undefined behvaior? You first test that the value > >> is less than zero and if it is less than zero you use it as a shift count. > > > > I'm not using trailing_shift as the shift count, I'm using shiftval: > > > > + /* Now we want to shift the previously generated constant into the > > + high half. */ > > + alt_codes[alt_cost - 2].code = ASHIFT; > > + alt_codes[alt_cost - 2].value = shiftval; > > + alt_codes[alt_cost - 2].use_uw = false; > > + alt_codes[alt_cost - 2].save_temporary = false; > I'm not referring to the generated code. The compiler itself will > exhibit undefined behavior due to the negative shift count in that test. >
Oh sorry. I get it now, the issue is with (loval << trailing_shift). I was trying to cover all possibilities, but if the trailing_shift is negative, the leading_shift should be positive and vice versa, so we could keep only the positive tests. I'll prepare a v2. Thanks, -- Raphael Moreira Zinsly
