IsCompileTimeConstant() (__builtin_constant_p) does not work in clang so no compile time computations of clampTo. |
|||
Issue description
base/numerics/safe_conversion has code that attempts to allow compile time conversions of numbers. That code depends on a function IsCompileTimeConstant() which in turn uses __builtin_constant_p. The problem is that __builtin_constant_p only returns true for literal constants. Not constexpr constants, and IsCompileTimeConstant is calling it on a function parameter which is not a literal constant.
I found this when I tried to apply the same method to Blink code that has an assembler version for Arm32.
And interesting test program to verify this is:
template <typename T>
constexpr bool IsCompileTimeConstant(const T v) {
return __builtin_constant_p(v);
}
int add_one(int);
constexpr int AddOne(const int i) {
if (IsCompileTimeConstant(i))
return i + 1;
return add_one(i);
}
// This is fine
static_assert(__builtin_constant_p(1), "");
// This will trigger a compile time assert
static_assert(IsCompileTimeConstant(1), "");
// This will not compile
static_assert(AddOne(1) == 2, "");
int add_two(int);
constexpr int AddTwo(int i) {
if (__builtin_constant_p(i))
return i + 2;
return add_two(i); // This will not compile.
}
,
Jul 23
I used the compiler in the Chromium tree. A recent clang. I tried both with and without optimization flags. This was a minimized test from some code that made all bots red so it looked like it affected both debug and release and android and desktop, but maybe they are all the same compiler. When I tried by hand I didn't use any special compiler flags in case that matters. Only -std=cxx17 and -O3.
,
Jul 23
It looks like this form of constexpr evaluation is supported in GCC, but not currently in Clang. I can't find whatever gave me the impression that a fix was underway for Clang, but I did find a more recent (and unfortunately stalled) CL to add LLVM support: https://reviews.llvm.org/D35190 I do remember disassembling the output and verifying that I got the expected behavior. So, I'm curious if this code gets folded anyway at more aggressive optimization levels, but not before the pass where the static_assert gets evaluated. Or, maybe I'm just remembering the GCC output.
,
Jul 24
Really looks like that patch has been forgotten. Nobody said there was anything wrong with it. Do you know someone in the clang project that can take a look maybe?
,
Jul 24
Yeah, I followed up with the clang people yesterday and got pointed to this bug: https://bugs.llvm.org/show_bug.cgi?id=4898#c38 I've asked thakis to help me find someone who would be willing to implement the functionality described in that bug. I'll add an update here assuming we get a taker.
,
Jul 25
Bug 829143 is also about __builtin_constant_p but in the ChromeOS toolchain. I hope you can find someone. It doesn't sound impossible to fix in clang and it's a bit disappointing that it doesn't do what you would expect.
,
Aug 3
This bug has an owner, thus, it's been triaged. Changing status to "assigned". |
|||
►
Sign in to add a comment |
|||
Comment 1 by jsc...@chromium.org
, Jul 23