[PATCH][X86] __builtin_ctz/clz sometimed defined for zero input

[Andrea Di Biagio]

On Fri, Oct 24, 2014 at 11:45 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> On Fri, Oct 24, 2014 at 5:15 AM, Andrea Di Biagio
> <andrea.dibiagio at gmail.com> wrote:
>>
>> Hi Sean,
>>
>> On Fri, Oct 24, 2014 at 3:02 AM, Sean Silva <chisophugis at gmail.com> wrote:
>> >
>> > If I understand correctly, this patch is trying to change the meaning of
>> > __builtin_ctz (et al.) under some extremely specific conditions. I don't
>> > think that is the right direction since it will cause surprising
>> > undefined
>> > behavior bugs across platforms.
>>
>> Paul's fix would only affect x86 behavior.
>> What it does is simply that on some x86 cpus, ctz/clz is defined on
>> zero (i.e. we have an instruction for it).
>> I don't think it can cause surprising undefined behavior across platforms.
>>
>> > The intrinsic is documented to have
>> > undefined behavior in the 0 case (everywhere I looked, including our
>> > internal docs); a user that relies on the 0 case has a bug. It would be
>> > nice
>> > to add a UBSan check for this undefined behavior though to help users
>> > fix
>> > their code.
>>
>> The intrinsic is documented to have the following behavior
>> (http://llvm.org/docs/LangRef.html#llvm-cttz-intrinsic):
>> "If src == 0 then the result is the size in bits of the type of src if
>> is_zero_undef == 0 and undef otherwise."
>> Where 'is_zero_undef' is the second argument to the intrinsic.
>> So, the intrinsic is not documented to have always undefined behavior
>> in the zero case.
>> Also I don't think that relying on the 0 case is a bug. It is a very
>> reasonable assumption on all modern x86 architectures.
>
>
> If the compiler's documentation says "X is undefined", it is a bug to rely
> on X. Period. There is a contract between the compiler and the user which is
> essential for the compiler to remain maintainable. The contract is the
> stated semantics of source code constructs (described in language standards
> and/or the compiler's documentation). You are conflating changing the
> contract with violating the contract. It is definitely a bug if a user's
> source code depends on behavior that violates the contract. It is a
> completely separate discussion as to whether or not a particular change is a
> reasonable modification of the contract that the compiler developers and
> users can agree on.
>

My bad. I was only looking at the semantic of the llvm cttz/ctlz intrinsics.
As you said, __builtin_clz/ctz is always undefined on zero. Relying on
that undefined is clearly wrong. Sorry for the confusion.

I definitely agree with you that we should try to generate optimal
code in the presence of a guard for the 0 case.
A guard for 0 followed by a call to the intrinsic ctz/clz with
"undefined at zero" semantic could be simplified to a call to
intrinsic ctz/clz that sets the "defined at zero" flag.