search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

[X86] clang::targets::X86TargetInfo::isCLZForZeroUndef should be BMI/LZCNT aware #39186

Closed Quuxplusone closed 5 years ago

Quuxplusone commented 5 years ago
Bugzilla Link PR40214
Status RESOLVED INVALID
Importance P enhancement
Reported by Simon Pilgrim (llvm-dev@redking.me.uk)
Reported on 2019-01-03 07:43:33 -0800
Last modified on 2019-01-03 13:54:16 -0800
Version trunk
Hardware PC Windows NT
CC andrea.dibiagio@gmail.com, craig.topper@gmail.com, filcab@gmail.com, llvm-bugs@lists.llvm.org, llvm-dev@redking.me.uk, spatel+llvm@rotateright.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also 
On x86 targets with BMI and LZCNT we should be able to return false from
isCLZForZeroUndef().

This would help avoid an issue we're seeing with ubsan warning that we have
zero inputs to clz/ctz in cases like:

#include <stdint.h>
uint64_t foo(uint64_t a0) {
    uint64_t r0 = static_cast<uint64_t>( __builtin_clzll( a0 ) );
    r0 = ( a0 ? r0 : 64 );
    return r0;
}
Quuxplusone commented 5 years ago

Do we really want ubsan behavior to be CPU specific? Shouldn't the builtin have one semantic no matter what? I already thought it was weird that it was different on other triples.

Quuxplusone commented 5 years ago

An alternative would be to provide clz/ctz intrinsics that support zero inputs

Quuxplusone commented 5 years ago

Can ubsan be changed to poison the result instead of the operation? Is executing the bulitin undefined behavior or is consuming the result undefined?

Quuxplusone commented 5 years ago 
(In reply to Craig Topper from comment #3)
> Can ubsan be changed to poison the result instead of the operation? Is
> executing the bulitin undefined behavior or is consuming the result
> undefined?

Sanjay - any ideas on this?

We have at least 3 options:

1 - use isCLZForZeroUndef

2 - create bmi tzcnt/lzcnt ia32 builtin intrinsics and have CGBuiltin emit
'zero is safe' variants of the generic intrinsic

3 - sanity check the input as well as the output (current codegen is pretty
bad: https://gcc.godbolt.org/z/ohAqTc) and try to strip that back out.
Quuxplusone commented 5 years ago

We already have bmi and lzcnt ia32 builtins.

Quuxplusone commented 5 years ago 
(In reply to Craig Topper from comment #5)
> We already have bmi and lzcnt ia32 builtins.

Dammit, thanks - this thing has been driving me nuts and it was all over a dumb
typo - __builtin_clzll vs __builtin_ia32_lzcnt_u64!
Quuxplusone commented 5 years ago 
(In reply to Simon Pilgrim from comment #4)
> (In reply to Craig Topper from comment #3)
> > Can ubsan be changed to poison the result instead of the operation? Is
> > executing the bulitin undefined behavior or is consuming the result
> > undefined?
>
> Sanjay - any ideas on this?

Clang doesn't actually have any documentation for these AFAICT, so I assume we
go by the GCC docs:
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
"If x is 0, the result is undefined."

So I read that as consuming the result is UB, but executing the builtin is not
UB. If that's not true, then the builtin/intrinsic would not be speculatable.

If the solution relies on that interpretation, we should run it by the clang/UB
experts to see if they agree.