-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[clang] Avoid printing overly large integer/_BitInt numbers in static assertion failure diagnostics #71675 #145053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
db0a3bd
to
8589275
Compare
@llvm/pr-subscribers-llvm-adt @llvm/pr-subscribers-clang Author: None (maramatias) ChangesResolves #71675 Performance comparison between origin code and patched code compiling on a RHEL9.6VM with 8cores and 16GB RAM
using Original code Patched code Full diff: https://github.com/llvm/llvm-project/pull/145053.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 96477ef6ddc9a..5a3e234ea1bcd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -642,6 +642,10 @@ Improvements to Clang's diagnostics
#GH69470, #GH59391, #GH58172, #GH46215, #GH45915, #GH45891, #GH44490,
#GH36703, #GH32903, #GH23312, #GH69874.
+- Improved the performance of static assertions envolving large integers by
+ using hex format instead of string.
+
+ Fixes #GH71675
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 16645ecf411e5..c45eafeca5f04 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -47,6 +47,7 @@
#include "clang/Sema/SemaOpenMP.h"
#include "clang/Sema/Template.h"
#include "clang/Sema/TemplateDeduction.h"
+#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
@@ -17575,7 +17576,21 @@ static bool ConvertAPValueToString(const APValue &V, QualType T,
break;
}
}
- V.getInt().toString(Str);
+
+ llvm::APSInt vInt = V.getInt();
+ if (llvm::APSInt::compareValues(
+ vInt, llvm::APSInt::getUnsigned(
+ std::numeric_limits<uint64_t>::max())) >= 0 ||
+ vInt < std::numeric_limits<int64_t>::min()) {
+ // The value of cutSize is not special, it is just a number of
+ // characters that gives us enough info without losing readability
+ const int cutSize = 20;
+ vInt.toString(Str, 16);
+ Str.erase(Str.begin() + cutSize, Str.end() - cutSize);
+ Str.insert(Str.begin() + cutSize, 3, '.');
+ } else {
+ vInt.toString(Str);
+ }
}
break;
diff --git a/clang/test/AST/ByteCode/intap.cpp b/clang/test/AST/ByteCode/intap.cpp
index 3f952ddf626b5..c32b8d67baf8b 100644
--- a/clang/test/AST/ByteCode/intap.cpp
+++ b/clang/test/AST/ByteCode/intap.cpp
@@ -79,12 +79,12 @@ typedef unsigned __int128 uint128_t;
static const __uint128_t UINT128_MAX =__uint128_t(__int128_t(-1L));
static_assert(UINT128_MAX == -1, "");
static_assert(UINT128_MAX == 1, ""); // both-error {{static assertion failed}} \
- // both-note {{'340282366920938463463374607431768211455 == 1'}}
+ // both-note {{'FFFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 1'}}
static const __int128_t INT128_MAX = UINT128_MAX >> (__int128_t)1;
static_assert(INT128_MAX != 0, "");
static_assert(INT128_MAX == 0, ""); // both-error {{failed}} \
- // both-note {{evaluates to '170141183460469231731687303715884105727 == 0'}}
+ // both-note {{evaluates to '7FFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 0'}}
static const __int128_t INT128_MIN = -INT128_MAX - 1;
@@ -105,14 +105,14 @@ namespace i128 {
static const __uint128_t UINT128_MAX =__uint128_t(__int128_t(-1L));
static_assert(UINT128_MAX == -1, "");
static_assert(UINT128_MAX == 1, ""); // both-error {{static assertion failed}} \
- // both-note {{'340282366920938463463374607431768211455 == 1'}}
+ // both-note {{'FFFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 1'}}
constexpr uint128_t TooMuch = UINT128_MAX * 2;
static const __int128_t INT128_MAX = UINT128_MAX >> (__int128_t)1;
static_assert(INT128_MAX != 0, "");
static_assert(INT128_MAX == 0, ""); // both-error {{failed}} \
- // both-note {{evaluates to '170141183460469231731687303715884105727 == 0'}}
+ // both-note {{evaluates to '7FFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 0'}}
constexpr int128_t TooMuch2 = INT128_MAX * INT128_MAX; // both-error {{must be initialized by a constant expression}} \
// both-note {{value 28948022309329048855892746252171976962977213799489202546401021394546514198529 is outside the range of representable}}
diff --git a/clang/test/Sema/enum.c b/clang/test/Sema/enum.c
index 01e41d4ebe956..80315a0c29b6a 100644
--- a/clang/test/Sema/enum.c
+++ b/clang/test/Sema/enum.c
@@ -196,7 +196,7 @@ enum GH59352 { // expected-warning {{enumeration values exceed range of largest
BigVal = 66666666666666666666wb
};
_Static_assert(BigVal == 66666666666666666666wb); /* expected-error {{static assertion failed due to requirement 'BigVal == 66666666666666666666wb'}}
- expected-note {{expression evaluates to '11326434445538011818 == 66666666666666666666'}}
+ expected-note {{expression evaluates to '11326434445538011818 == 39D2F941E420AAAAA<U+0000><U+0000><U+0000>...<U+0000><U+0000><U+0000>39D2F941E420AAAAA'}}
*/
_Static_assert(
_Generic(BigVal, // expected-error {{static assertion failed}}
diff --git a/clang/test/SemaCXX/static-assert.cpp b/clang/test/SemaCXX/static-assert.cpp
index bf6a2eeb432a3..4b9d23cb8c808 100644
--- a/clang/test/SemaCXX/static-assert.cpp
+++ b/clang/test/SemaCXX/static-assert.cpp
@@ -255,6 +255,12 @@ int f() {
}
}
+namespace GH71675 {
+constexpr unsigned _BitInt(__BITINT_MAXWIDTH__ >> 6) F = ~0; // expected-warning {{Clang extension}}
+static_assert(F == 1,""); // expected-error {{static assertion failed due to requirement 'F == 1'}} \
+ // expected-note {{expression evaluates to 'FFFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 1'}}
+} // namespace GH71675
+
namespace Diagnostics {
/// No notes for literals.
static_assert(false, ""); // expected-error {{failed}}
|
clang/docs/ReleaseNotes.rst
Outdated
@@ -647,7 +647,11 @@ Improvements to Clang's diagnostics | |||
#GH69470, #GH59391, #GH58172, #GH46215, #GH45915, #GH45891, #GH44490, | |||
#GH36703, #GH32903, #GH23312, #GH69874. | |||
|
|||
|
|||
- Improved the performance of static assertions envolving large integers by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Improved the performance of static assertions envolving large integers by | |
- Improved the performance of static assertions involving large integers by |
clang/docs/ReleaseNotes.rst
Outdated
@@ -647,7 +647,11 @@ Improvements to Clang's diagnostics | |||
#GH69470, #GH59391, #GH58172, #GH46215, #GH45915, #GH45891, #GH44490, | |||
#GH36703, #GH32903, #GH23312, #GH69874. | |||
|
|||
|
|||
- Improved the performance of static assertions envolving large integers by | |||
using hex format instead of string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using hex format instead of string. | |
using hex format instead of string for particularly large values. |
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
std::numeric_limits<uint64_t>::max())) >= 0 || | ||
vInt < std::numeric_limits<int64_t>::min()) { | ||
// The value of cutSize is not special, it is just a number of | ||
// characters that gives us enough info without losing readability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// characters that gives us enough info without losing readability | |
// characters that gives us enough info without losing readability. |
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
vInt < std::numeric_limits<int64_t>::min()) { | ||
// The value of cutSize is not special, it is just a number of | ||
// characters that gives us enough info without losing readability | ||
const int cutSize = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const int cutSize = 20; | |
constexpr int cutOffSize = 20; |
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
|
||
llvm::APSInt vInt = V.getInt(); | ||
if (llvm::APSInt::compareValues( | ||
vInt, llvm::APSInt::getUnsigned( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole bit here is equivilent to: isRepresentableByInt64
clang/lib/Sema/SemaDeclCXX.cpp
Outdated
// characters that gives us enough info without losing readability | ||
const int cutSize = 20; | ||
vInt.toString(Str, 16); | ||
Str.erase(Str.begin() + cutSize, Str.end() - cutSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here doesn't really seem right. We're just erasing blindly in the middle, despite not knowing that we have enough room. I think what we REALLY want is to decide to keep the first N characters, and the last N characters (and still do the ...).
Additionally, I think instead of doing the whole to-string mechanism here (and since we're doing it in hex it is easy to do so), we could just do a numeric-trim of hte front/back instead and join them afterwards.
THAT SAID: I'm not sold on doing this as a hex-print, I think we can keep it decimal. We could have a new function in APInt
that does a print only the first N and last N, with dots between
kind of thing. If we have that, we don't actually have to do any branching here, because choosing a reasonable number there plus decent algorithm for it (basically, normal RHS processing until we reach 'N' digits, then figure out how many orders of magnitude to divide by using 'log' to figure out how to get the remaining 'N').
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of keeping the decimals. I think it avoids the weirdness happening in the enum test, which I think might have been caused by some characters getting cut in half. Working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi just wanted to run something by you before I make further changes.
Before, we were using the value of the APInt to decide if we would truncate or not. I am tempted since we will be looking at the number of digits anyway to decide how many to skip to instead use that as the deciding factor, so something like, if the number of digits in the APInt value is larger than 40 (This is if we want to keep N=20) then we truncate keeping the first and last 20.
Were you thinking of some other way to decide what N should be by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know how this is supposed to work. We need to know what to display without printing the value to a string at all, because THAT is what's breaking our neck. The simplest solution would be to just check the value (or even getActiveBits()
?) and if that is larger than some arbitrary value (you can just check that printing the note doesn't take longer than, say, 3 seconds), we simply skip the note altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi just wanted to run something by you before I make further changes. Before, we were using the value of the APInt to decide if we would truncate or not. I am tempted since we will be looking at the number of digits anyway to decide how many to skip to instead use that as the deciding factor, so something like, if the number of digits in the APInt value is larger than 40 (This is if we want to keep N=20) then we truncate keeping the first and last 20. Were you thinking of some other way to decide what N should be by the way?
I had no problem with the "around 20 digits" on each side. My suggestion was to have the actual printing in APInt
do the magic (via separate function). So algorithmically, it is something like:
unsigned end_digits_printed = 0;
// Print the 'end digits'.
while(value != 0 && end_digits_printed <20) {
APInt temp;
int64_t rem;
APInt::udivrem(value, 10, temp, rem);//udiv becasue before this we figured out 'negative' and made it positive.
//add-rem-to-buffer
value = temp;
}
// print the '...'
if (value == 0) return;
// This gives more of a log-base-16 by dividing it by 4 I think (or should this be a multiply?), but gives us an approximation.
unsigned ApproxDigitsLeft = value.logBase2() / 4;
if (ApproxDigitsLeft > 20)
value = value.udiv( 10 * (ApproxDigitsLeft - 20)); // removes the 'middle' approx all-but-20.
// print the leading part.
while (value != 0) {
// same stuff as above.
}
So basically, we don't have to calculate the whole thing to figure out how many to remove, but we can be 'reasonably accurate' here. WDYT? ALSO, the base-16 to base-10 approximation might need some tweeking so that we always print some of the leading digits (though I think 'at least 20 hex digits' is... a lot to make sure of that.). We MIGHT want to make the top-half a little smaller since there is a hex->decimal relationship, so maybe make sure we're printing the top 16 hex-digit-worth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, that is basically what I had in mind. I'm extending toString and there is a Pos variable as you have there counting how many digits were already inserted and we can get the number of remaing using value.logBase2/log2(Radix) +1 that actually gives the exact count. Then the division will be value.udiv( Radix ^ (ApproxDigitsLeft - 20))
. I just need to get that pow working it's throwing a seg fault but I'll figure that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tbaederr I think the last comment from @erichkeane probably made it clearer but basically what we were discussing is creating a separate version of toString (tentatively naming it toStringTruncated) that does not print all of the number, the implementation of that function basically takes the number and does a huge number of divisions to get each digit in the resulting String, the approach suggested is that once we have printed 20 digits we do a division that jumps us to the first 20. I'm hoping I can have something to push in the next couple days time permitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can get the number of remaing using value.logBase2/log2(Radix) +1 that actually gives the exact count.
That sounds VAGUELY familiar :) Its been a few decades since I've been in a math class....
With the 'pow', you probably need SOME sort of guard to make sure you don't exceed the number we can store.
clang/test/Sema/enum.c
Outdated
@@ -196,7 +196,7 @@ enum GH59352 { // expected-warning {{enumeration values exceed range of largest | |||
BigVal = 66666666666666666666wb | |||
}; | |||
_Static_assert(BigVal == 66666666666666666666wb); /* expected-error {{static assertion failed due to requirement 'BigVal == 66666666666666666666wb'}} | |||
expected-note {{expression evaluates to '11326434445538011818 == 66666666666666666666'}} | |||
expected-note {{expression evaluates to '11326434445538011818 == 39D2F941E420AAAAA<U+0000><U+0000><U+0000>...<U+0000><U+0000><U+0000>39D2F941E420AAAAA'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems VERY wrong here...
What are the times with the full |
… assertion failure diagnostics llvm#71675
416d965
to
f58ea20
Compare
@tbaederr Apologies for the delay. I re-ran the original code, compiling the same sample program but without the right shift, left it running over 30 minutes (clock time) it was no able to complete compilation it printed the error but not the note. |
… static assertion failure diagnostics llvm#71675
@@ -647,7 +647,11 @@ Improvements to Clang's diagnostics | |||
#GH69470, #GH59391, #GH58172, #GH46215, #GH45915, #GH45891, #GH44490, | |||
#GH36703, #GH32903, #GH23312, #GH69874. | |||
|
|||
|
|||
- Improved the performance of static assertions involving large integers by | |||
using hex format instead of string for particularly large values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still using hex?
|
||
- Improved the performance of static assertions involving large integers by | ||
using hex format instead of string for particularly large values. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do a newline here I think? If you do it separates the things during rendering.
@@ -87,12 +87,12 @@ typedef unsigned __int128 uint128_t; | |||
static const __uint128_t UINT128_MAX =__uint128_t(__int128_t(-1L)); | |||
static_assert(UINT128_MAX == -1, ""); | |||
static_assert(UINT128_MAX == 1, ""); // both-error {{static assertion failed}} \ | |||
// both-note {{'340282366920938463463374607431768211455 == 1'}} | |||
// both-note {{'FFFFFFFFFFFFFFFFFFFF...FFFFFFFFFFFFFFFFFFFF == 1'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is making these print in hex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was not fixed yet I was hitting a problem where the division was throwing a Floting point error that I was still looking at.
@@ -2278,8 +2285,28 @@ void APInt::toString(SmallVectorImpl<char> &Str, unsigned Radix, bool Signed, | |||
} | |||
} else { | |||
int Pos = 0; | |||
// The value of cutOffSize is not special, it is just a number of | |||
// characters that gives us enough info without losing readability. | |||
constexpr int cutOffSize = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an argument?
@@ -2164,6 +2164,13 @@ void APInt::fromString(unsigned numbits, StringRef str, uint8_t radix) { | |||
void APInt::toString(SmallVectorImpl<char> &Str, unsigned Radix, bool Signed, | |||
bool formatAsCLiteral, bool UpperCase, | |||
bool InsertSeparators) const { | |||
toStringTruncated(Str, Radix, Signed, false, formatAsCLiteral, UpperCase, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS there ANY perf hit whatsoever for doing this? We're doing quite a bit of work on what is perhaps a hot-ish path, can you run the compiler perf tester thing on this? @Endilll knows how if you don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never run them. @Endhill if you could give me some pointers, appreciate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See "Pre-Commit Measurements" section at https://llvm-compile-time-tracker.com/about.php
while (Tmp.getBoolValue()) { | ||
uint64_t Digit; | ||
if (truncate && Pos == cutOffSize) { | ||
unsigned numDigits = (int32_t)(Tmp.logBase2()/log2(Radix))+1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsigned numDigits = (int32_t)(Tmp.logBase2()/log2(Radix))+1; | |
unsigned numDigits = static_cast<int32_t>(Tmp.logBase2()/log2(Radix))+1); |
ALSO: Why cast to int32, but then assign to an unsigned? Should we be doing math as unsigned? or as signed?
Also, a comment on what this is doing could be helpful.
Also... is there value to pre-calculating log2(Radix)
and keeping it somewhere so we don't re-evaluate it every time? or is log2
pure
?
Str.append(3,'.'); | ||
} | ||
else { | ||
Str.append(3,'.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any way to find the 'first' digit or something in this case? I find myself wondering if there is a 'better' way here...
IS there any special-casing we can do here for base-10? I DO find myself wondering if pow
is the best way for Radix==10. I know that V<<3 + v << 1 is the same as *10, I wonder if someone better at math than I could come up with a way to apply that as a way to do the 10^N as a simple set of shifts/math?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not 100% happy with that else either so I have been thinking about this.
I could not find any way to do it with just shifts also because if we want to support what toString supports, which I think makes sense, the else branch which does divisions instead of shift is for base 10 and base 36.
I had one alternative idea but I was still checking to see if it would work, the property I am trying to check is that if we take a word worth of bites in one of these multiword APInt we can work with it in isolation I mean that there is no digit that is half in one word half in the other.
The idea was that instead of looking at the number of digits already printed we could take a word worth of bites and use the usual approach that uses division to get some number of digit one by one. After that if we were truncating we would shift log2(getActiveWords-2*wordlenght)
(might be -1 at that point rather than -2 actually). The idea is this should put us at the first active word, there we would continue using divisions to get the digits. This would give with the default word size some 9/10 digits from the end, and some arbitrary number from the beginning but at most again 9/10. Does it sound like this could work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sounds like it could work? I'm not sure of the size implications...
One thing to note, 'exactly 20' numbers in the 1st number isn't important. What IS important is 'enough of the first few that a couple of patterns are obvious', so the numbers have to be accurate, but the number OF them isn't important.
I'm not sure I have a better idea, but I'm still thinking :) Have an attempt at the above and see if it works out.
Resolves #71675
Performance comparison between origin code and patched code compiling on a RHEL9.6VM with 8cores and 16GB RAM
compiling sample program
using
./llvm-project/build/bin/clang++ ut71675.cpp -o ut71675
Original code
user avg over 10 runs - 1.6748
sys avg over 10 runs - 0.0299
Patched code
user avg over 10 runs - 0.2149
sys avg over 10 runs - 0.0198