-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[BranchFolding] Remove dubious assert from operator< #71639
Conversation
`MergePotentialElts::operator<` asserts that the two elements being compared are not equal. However, sorting functions are allowed to invoke the comparison function with equal arguments (though they usually don't for efficiency reasons). There is an existing special-case that disables the assert if _GLIBCXX_DEBUG is used, which may invoke the comparator with equal args to verify strict weak ordering. I believe libc++ also has strict weak ordering checks under some options nowadays. Most recently llvm#71312 was reported, where a change to glibc's qsort_r implementation can also result in comparison between equal elements. From what I understood this is an inefficiency that will be fixed on the glibc side as well, but I think at this point we should just remove this assertion. Fixes llvm#71312.
// _GLIBCXX_DEBUG checks strict weak ordering, which involves comparing | ||
// an object with itself. | ||
#ifndef _GLIBCXX_DEBUG | ||
llvm_unreachable("Predecessor appears twice"); | ||
#else | ||
return false; | ||
#endif |
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 see the point of asserting this in the first place
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 the point is to ensure that the array being sorted does not contain a pair of elements that compare equal, in which case it's more efficient to verify that by inspecting the sorted array:
for (MPIterator i = MergePotentials.begin(), n = std::next(i);
n != MergePotentials.end();
i = n, n++)
assert(*i < *n);
Do we need to backport this to release/17.x ? |
We don't know yet whether the glibc workaround is effective. In any case, it's something that is likely to impact other systems as well. |
Unfortunate that this was merged without acknowledging my response to @arsenm about the logic of the assert. |
`MergePotentialElts::operator<` asserts that the two elements being compared are not equal. However, sorting functions are allowed to invoke the comparison function with equal arguments (though they usually don't for efficiency reasons). There is an existing special-case that disables the assert if _GLIBCXX_DEBUG is used, which may invoke the comparator with equal args to verify strict weak ordering. I believe libc++ also has strict weak ordering checks under some options nowadays. Recently, #71312 was reported, where a change to glibc's qsort_r implementation can also result in comparison between equal elements. From what I understood, this is an inefficiency that will be fixed on the glibc side as well, but I think at this point we should just remove this assertion. Fixes #71312. (cherry picked from commit 74a76a2)
MergePotentialElts::operator<
asserts that the two elements being compared are not equal. However, sorting functions are allowed to invoke the comparison function with equal arguments (though they usually don't for efficiency reasons).There is an existing special-case that disables the assert if _GLIBCXX_DEBUG is used, which may invoke the comparator with equal args to verify strict weak ordering. I believe libc++ also has strict weak ordering checks under some options nowadays.
Recently, #71312 was reported, where a change to glibc's qsort_r implementation can also result in comparison between equal elements. From what I understoo,d this is an inefficiency that will be fixed on the glibc side as well, but I think at this point we should just remove this assertion.
Fixes #71312.