[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4553727 , @aaron.ballman 
wrote:

> In D155809#4553694 , @danlark wrote:
>
>> In D155809#4553690 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4551876 , @danlark 
>>> wrote:
>>>
 In D155809#4551721 , @cor3ntin 
 wrote:

> I think the test we want is a set of classes and virtuaal fuc
>
> In D155809#4551686 , @philnik 
> wrote:
>
>> @aaron.ballman I think what @danlark means is that when building clang 
>> against a libc++ which has debug assertions enabled, the clang tests he 
>> mentioned result in an assertion firing within libc++. i.e. the libc++ 
>> debug mode catches the non-strict weak ordering that it gets from the 
>> clang code. That's why he can't just add a test that fires the 
>> assertion. Currently, there are no bots that build clang against a 
>> libc++ with debug assertions enabled.
>
> What we are asking for is a standalone minimal test that doesn't depend 
> on libc++ at all, living in `clang/test/`

 That's impossible. No test will fail because no implementation of 
 std::stable_sort calls `comp(a, a)`. Only libcxx calls it in debug mode. 
 But by C++ standard rules, `comp(a, a)` is allowed to be called.
>>>
>>> It's not impossible. :-) Can you share a link to which libc++ test case is 
>>> failing in debug mode? We can work backwards from there to build up a test 
>>> case.
>>
>> The issue is not in libc++, the assert fires if you build clang in the 
>> following way:
>>
>> 1. Build clang binary from source with the latest libc++ at head.
>> 2. Build clang binary from source with -D_LIBCPP_ENABLE_DEBUG_MODE=1 
>> -D_LIBCPP_ENABLE_HARDENED_MODE=1 (that's a libc++ define controlling 
>> standard library behavior which every file in the clang project uses, 
>> including this piece of code)
>> 3. Run llvm-lit on clang tests
>> 4. You will see 2 failures in  
>> llvm/llvm-project/clang/test/CodeGen:available-externally-hidden.cpp.test 
>> and 
>> llvm/llvm-project/clang/test/CodeGenCXX:cxx2a-three-way-comparison.cpp.test (
>>
>> Without steps 1 and 2, there is no way to have assert on line 1569 
>> .
>>  No implementation of std::stable_sort in any major standard library can 
>> reach this line
>
> OH! The critical bit I was missing that you're building *Clang with libc++* 
> in order to see this issue, and the problem is that this particular call to 
> `std::stable_sort` has UB because of a violated precondition. Thanks for 
> sticking with the discussion until we had this clarity.
>
> The changes LGTM now that I understand the context better. This doesn't seem 
> to warrant a release note though, as users wouldn't see any differences.

Thank you, I'll improve my communication so that we reach to an agreement 
faster.

I don't have commit rights. Danila Kutenin. kutdan...@yandex.ru


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4553690 , @aaron.ballman 
wrote:

> In D155809#4551876 , @danlark wrote:
>
>> In D155809#4551721 , @cor3ntin 
>> wrote:
>>
>>> I think the test we want is a set of classes and virtuaal fuc
>>>
>>> In D155809#4551686 , @philnik 
>>> wrote:
>>>
 @aaron.ballman I think what @danlark means is that when building clang 
 against a libc++ which has debug assertions enabled, the clang tests he 
 mentioned result in an assertion firing within libc++. i.e. the libc++ 
 debug mode catches the non-strict weak ordering that it gets from the 
 clang code. That's why he can't just add a test that fires the assertion. 
 Currently, there are no bots that build clang against a libc++ with debug 
 assertions enabled.
>>>
>>> What we are asking for is a standalone minimal test that doesn't depend on 
>>> libc++ at all, living in `clang/test/`
>>
>> That's impossible. No test will fail because no implementation of 
>> std::stable_sort calls `comp(a, a)`. Only libcxx calls it in debug mode. But 
>> by C++ standard rules, `comp(a, a)` is allowed to be called.
>
> It's not impossible. :-) Can you share a link to which libc++ test case is 
> failing in debug mode? We can work backwards from there to build up a test 
> case.

The issue is not in libc++, the assert fires if you build clang in the 
following way:

1. Build clang binary from source with the latest libc++ at head.
2. Build clang binary from source with -D_LIBCPP_ENABLE_DEBUG_MODE=1 
-D_LIBCPP_ENABLE_HARDENED_MODE (that's a libc++ define controlling standard 
library behavior which every file in the clang project uses, including this 
piece of code)
3. Run llvm-lit on clang tests
4. You will see 2 failures in  
llvm/llvm-project/clang/test/CodeGen:available-externally-hidden.cpp.test and 
llvm/llvm-project/clang/test/CodeGenCXX:cxx2a-three-way-comparison.cpp.test (

Without steps 1 and 2, there is no way to have assert on line 1569 
.
 No implementation of std::stable_sort in any major standard library can reach 
this line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4551721 , @cor3ntin wrote:

> I think the test we want is a set of classes and virtuaal fuc
>
> In D155809#4551686 , @philnik wrote:
>
>> @aaron.ballman I think what @danlark means is that when building clang 
>> against a libc++ which has debug assertions enabled, the clang tests he 
>> mentioned result in an assertion firing within libc++. i.e. the libc++ debug 
>> mode catches the non-strict weak ordering that it gets from the clang code. 
>> That's why he can't just add a test that fires the assertion. Currently, 
>> there are no bots that build clang against a libc++ with debug assertions 
>> enabled.
>
> What we are asking for is a standalone minimal test that doesn't depend on 
> libc++ at all, living in `clang/test/`

That's impossible. No test will fail because no implementation of 
std::stable_sort calls `comp(a, a)`. Only libcxx calls it in debug mode. But by 
C++ standard rules, `comp(a, a)` is allowed to be called.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4550711 , @aaron.ballman 
wrote:

> In D155809#4550700 , @danlark wrote:
>
>> In D155809#4550682 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4550674 , @danlark 
>>> wrote:
>>>
 In D155809#4550663 , 
 @aaron.ballman wrote:

> In D155809#4550654 , @danlark 
> wrote:
>
>> In D155809#4550646 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4527890 , @danlark 
>>> wrote:
>>>
 In D155809#4527847 , 
 @aaron.ballman wrote:

> In D155809#4527199 , 
> @danlark wrote:
>
>> In D155809#4521494 , 
>> @rsmith wrote:
>>
>>> This looks correct to me, but it's still a little subtle. Perhaps 
>>> it'd be clearer to map the method to an integer (0 for copy 
>>> assignment, 1 for move assignment, 2 for destructor, 3 for equality 
>>> comparison), and then order them by that integer? That'd be more 
>>> obviously a strict weak order.
>>
>>
>>
>> In D155809#4520765 , 
>> @shafik wrote:
>>
>>> I am not sure about this change but I think we at least need a test 
>>> and this does not seem non-functional if it prevents a crash.
>>
>> This is NFC as it only prevents further assert to fire when 
>> stable_sort compares the element with itself
>
> Richard's suggestion makes sense to me as a clarifying change to the 
> code. I also agree with Shafik -- if this prevents an assertion from 
> firing in practice, then it's a functional change that should come 
> with tests. Or are you saying the assertion isn't happening in 
> practice and this is a defensive change?

 The assertion happens in debug libcxx mode after 
 https://reviews.llvm.org/D150264.  This is a defensive change, in 
 practice, 2 same functions cannot happen in this comparator, this is 
 only for preventing assertions on line 1568 
 
>>>
>>> I apologize, but I'm still confused. If this assertion triggers in 
>>> practice in debug modes with libc++, we should be able to make a 
>>> stand-alone reproducer that we test as part of these changes within 
>>> Clang.
>>
>> This assertion triggers in debug mode for various tests but clang is not 
>> tested against libc++ debug mode for now. In non debug mode the 
>> assertion is impossible to reach because in practice comp(a, a) is not 
>> called for all implementations of sorting in all major standard libraries
>
> Okay, I think you should take the existing tests that trigger the 
> assertion and reduce the code down to just what's needed to trigger the 
> assertion, then add that code as a test case to Clang so that we can 
> demonstrate the assert happens before your patch and doesn't happen after 
> your patch. We've got a special lit mode (`// REQUIRES: asserts` as a 
> comment near the `RUN` line) to enable the test only in asserts builds so 
> you don't have to worry about assertions disabled changing the test 
> behavior.

 libc++ debug mode is different from assertions in LLVM main library (first 
 is controlled with -D_LIBCPP_ENABLE_DEBUG_MODE, second is with -UNDEBUG). 
 I claim that now I cannot write the test which fails in any existing CI 
 configuration. And I cannot add a new version of CI because of failing 
 tests. That's why I defensively clean up comparators to enable CI in more 
 modes. I made the change as easy as possible to prove that it does not 
 harm the sorting overall.
>>>
>>> If it fails in any libc++ CI pipeline, you should be able to craft the same 
>>> code to fail within Clang's test suite. The changes in the patch all look 
>>> correct to me; the problem is that the patch claims to be NFC when it's 
>>> not. It is making a functional change (it fixes assertions you were able to 
>>> hit) and it has no test coverage for that.
>>
>> @ldionne is it possible to run clang test suite in the libc++ debug mode? I 
>> looked through the CI and my opinion it's an exclusively libc++ feature 
>> 

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a subscriber: ldionne.
danlark added a comment.

In D155809#4550682 , @aaron.ballman 
wrote:

> In D155809#4550674 , @danlark wrote:
>
>> In D155809#4550663 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4550654 , @danlark 
>>> wrote:
>>>
 In D155809#4550646 , 
 @aaron.ballman wrote:

> In D155809#4527890 , @danlark 
> wrote:
>
>> In D155809#4527847 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4527199 , @danlark 
>>> wrote:
>>>
 In D155809#4521494 , @rsmith 
 wrote:

> This looks correct to me, but it's still a little subtle. Perhaps 
> it'd be clearer to map the method to an integer (0 for copy 
> assignment, 1 for move assignment, 2 for destructor, 3 for equality 
> comparison), and then order them by that integer? That'd be more 
> obviously a strict weak order.



 In D155809#4520765 , @shafik 
 wrote:

> I am not sure about this change but I think we at least need a test 
> and this does not seem non-functional if it prevents a crash.

 This is NFC as it only prevents further assert to fire when 
 stable_sort compares the element with itself
>>>
>>> Richard's suggestion makes sense to me as a clarifying change to the 
>>> code. I also agree with Shafik -- if this prevents an assertion from 
>>> firing in practice, then it's a functional change that should come with 
>>> tests. Or are you saying the assertion isn't happening in practice and 
>>> this is a defensive change?
>>
>> The assertion happens in debug libcxx mode after 
>> https://reviews.llvm.org/D150264.  This is a defensive change, in 
>> practice, 2 same functions cannot happen in this comparator, this is 
>> only for preventing assertions on line 1568 
>> 
>
> I apologize, but I'm still confused. If this assertion triggers in 
> practice in debug modes with libc++, we should be able to make a 
> stand-alone reproducer that we test as part of these changes within Clang.

 This assertion triggers in debug mode for various tests but clang is not 
 tested against libc++ debug mode for now. In non debug mode the assertion 
 is impossible to reach because in practice comp(a, a) is not called for 
 all implementations of sorting in all major standard libraries
>>>
>>> Okay, I think you should take the existing tests that trigger the assertion 
>>> and reduce the code down to just what's needed to trigger the assertion, 
>>> then add that code as a test case to Clang so that we can demonstrate the 
>>> assert happens before your patch and doesn't happen after your patch. We've 
>>> got a special lit mode (`// REQUIRES: asserts` as a comment near the `RUN` 
>>> line) to enable the test only in asserts builds so you don't have to worry 
>>> about assertions disabled changing the test behavior.
>>
>> libc++ debug mode is different from assertions in LLVM main library (first 
>> is controlled with -D_LIBCPP_ENABLE_DEBUG_MODE, second is with -UNDEBUG). I 
>> claim that now I cannot write the test which fails in any existing CI 
>> configuration. And I cannot add a new version of CI because of failing 
>> tests. That's why I defensively clean up comparators to enable CI in more 
>> modes. I made the change as easy as possible to prove that it does not harm 
>> the sorting overall.
>
> If it fails in any libc++ CI pipeline, you should be able to craft the same 
> code to fail within Clang's test suite. The changes in the patch all look 
> correct to me; the problem is that the patch claims to be NFC when it's not. 
> It is making a functional change (it fixes assertions you were able to hit) 
> and it has no test coverage for that.

@ldionne is it possible to run clang test suite in the libc++ debug mode? I 
looked through the CI and my opinion it's an exclusively libc++ feature 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4550663 , @aaron.ballman 
wrote:

> In D155809#4550654 , @danlark wrote:
>
>> In D155809#4550646 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4527890 , @danlark 
>>> wrote:
>>>
 In D155809#4527847 , 
 @aaron.ballman wrote:

> In D155809#4527199 , @danlark 
> wrote:
>
>> In D155809#4521494 , @rsmith 
>> wrote:
>>
>>> This looks correct to me, but it's still a little subtle. Perhaps it'd 
>>> be clearer to map the method to an integer (0 for copy assignment, 1 
>>> for move assignment, 2 for destructor, 3 for equality comparison), and 
>>> then order them by that integer? That'd be more obviously a strict weak 
>>> order.
>>
>>
>>
>> In D155809#4520765 , @shafik 
>> wrote:
>>
>>> I am not sure about this change but I think we at least need a test and 
>>> this does not seem non-functional if it prevents a crash.
>>
>> This is NFC as it only prevents further assert to fire when stable_sort 
>> compares the element with itself
>
> Richard's suggestion makes sense to me as a clarifying change to the 
> code. I also agree with Shafik -- if this prevents an assertion from 
> firing in practice, then it's a functional change that should come with 
> tests. Or are you saying the assertion isn't happening in practice and 
> this is a defensive change?

 The assertion happens in debug libcxx mode after 
 https://reviews.llvm.org/D150264.  This is a defensive change, in 
 practice, 2 same functions cannot happen in this comparator, this is only 
 for preventing assertions on line 1568 
 
>>>
>>> I apologize, but I'm still confused. If this assertion triggers in practice 
>>> in debug modes with libc++, we should be able to make a stand-alone 
>>> reproducer that we test as part of these changes within Clang.
>>
>> This assertion triggers in debug mode for various tests but clang is not 
>> tested against libc++ debug mode for now. In non debug mode the assertion is 
>> impossible to reach because in practice comp(a, a) is not called for all 
>> implementations of sorting in all major standard libraries
>
> Okay, I think you should take the existing tests that trigger the assertion 
> and reduce the code down to just what's needed to trigger the assertion, then 
> add that code as a test case to Clang so that we can demonstrate the assert 
> happens before your patch and doesn't happen after your patch. We've got a 
> special lit mode (`// REQUIRES: asserts` as a comment near the `RUN` line) to 
> enable the test only in asserts builds so you don't have to worry about 
> assertions disabled changing the test behavior.

libc++ debug mode is different from assertions in LLVM main library (first is 
controlled with -D_LIBCPP_ENABLE_DEBUG_MODE, second is with -UNDEBUG). I claim 
that now I cannot write the test which fails in any existing CI configuration. 
And I cannot add a new version of CI because of failing tests. That's why I 
defensively clean up comparators to enable CI in more modes. I made the change 
as easy as possible to prove that it does not harm the sorting overall.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4550646 , @aaron.ballman 
wrote:

> In D155809#4527890 , @danlark wrote:
>
>> In D155809#4527847 , 
>> @aaron.ballman wrote:
>>
>>> In D155809#4527199 , @danlark 
>>> wrote:
>>>
 In D155809#4521494 , @rsmith 
 wrote:

> This looks correct to me, but it's still a little subtle. Perhaps it'd be 
> clearer to map the method to an integer (0 for copy assignment, 1 for 
> move assignment, 2 for destructor, 3 for equality comparison), and then 
> order them by that integer? That'd be more obviously a strict weak order.



 In D155809#4520765 , @shafik 
 wrote:

> I am not sure about this change but I think we at least need a test and 
> this does not seem non-functional if it prevents a crash.

 This is NFC as it only prevents further assert to fire when stable_sort 
 compares the element with itself
>>>
>>> Richard's suggestion makes sense to me as a clarifying change to the code. 
>>> I also agree with Shafik -- if this prevents an assertion from firing in 
>>> practice, then it's a functional change that should come with tests. Or are 
>>> you saying the assertion isn't happening in practice and this is a 
>>> defensive change?
>>
>> The assertion happens in debug libcxx mode after 
>> https://reviews.llvm.org/D150264.  This is a defensive change, in practice, 
>> 2 same functions cannot happen in this comparator, this is only for 
>> preventing assertions on line 1568 
>> 
>
> I apologize, but I'm still confused. If this assertion triggers in practice 
> in debug modes with libc++, we should be able to make a stand-alone 
> reproducer that we test as part of these changes within Clang.

This assertion triggers in debug mode for various tests but clang is not tested 
against libc++ debug mode for now. In non debug mode the assertion is 
impossible to reach because in practice comp(a, a) is not called for all 
implementations of sorting in all major standard libraries


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4527847 , @aaron.ballman 
wrote:

> In D155809#4527199 , @danlark wrote:
>
>> In D155809#4521494 , @rsmith wrote:
>>
>>> This looks correct to me, but it's still a little subtle. Perhaps it'd be 
>>> clearer to map the method to an integer (0 for copy assignment, 1 for move 
>>> assignment, 2 for destructor, 3 for equality comparison), and then order 
>>> them by that integer? That'd be more obviously a strict weak order.
>>
>>
>>
>> In D155809#4520765 , @shafik wrote:
>>
>>> I am not sure about this change but I think we at least need a test and 
>>> this does not seem non-functional if it prevents a crash.
>>
>> This is NFC as it only prevents further assert to fire when stable_sort 
>> compares the element with itself
>
> Richard's suggestion makes sense to me as a clarifying change to the code. I 
> also agree with Shafik -- if this prevents an assertion from firing in 
> practice, then it's a functional change that should come with tests. Or are 
> you saying the assertion isn't happening in practice and this is a defensive 
> change?

The assertion happens in debug libcxx mode after 
https://reviews.llvm.org/D150264.  This is a defensive change, in practice, 2 
same functions cannot happen in this comparator, this is only for preventing 
assertions on line 1568 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4521494 , @rsmith wrote:

> This looks correct to me, but it's still a little subtle. Perhaps it'd be 
> clearer to map the method to an integer (0 for copy assignment, 1 for move 
> assignment, 2 for destructor, 3 for equality comparison), and then order them 
> by that integer? That'd be more obviously a strict weak order.



In D155809#4520765 , @shafik wrote:

> I am not sure about this change but I think we at least need a test and this 
> does not seem non-functional if it prevents a crash.

This is NFC as it only prevents further assert to fire when stable_sort 
compares the element with itself


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 542492.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809

Files:
  clang/lib/AST/VTableBuilder.cpp


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   std::stable_sort(
   NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
   [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+if (A == B)
+  return false;
 if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
   return A->isCopyAssignmentOperator();
 if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   std::stable_sort(
   NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
   [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+if (A == B)
+  return false;
 if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
   return A->isCopyAssignmentOperator();
 if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
Herald added a project: All.
danlark requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In sorting elements can compare with themselves and sometimes assert further 
down the line was triggered


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155809

Files:
  clang/lib/AST/VTableBuilder.cpp


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,7 @@
   std::stable_sort(
   NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
   [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+if (A == B) return false;
 if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
   return A->isCopyAssignmentOperator();
 if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,7 @@
   std::stable_sort(
   NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
   [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+if (A == B) return false;
 if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
   return A->isCopyAssignmentOperator();
 if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116037: [clang-include-fixer] Fix incorrect ranking because of dangling references

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
danlark added reviewers: alexfh, klimek.
Herald added subscribers: arphaman, mgrang.
danlark requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

SymbolAndSignals stores SymbolInfo which stores two std::strings. Then
the values are stored in a llvm::DenseMap. When
the sorting is happening, SymbolAndSignals are swapped and thus because
of small string optimization some strings may become invalid. This
results in incorrect ranking.

This was detected when running new std::sort algorithm against llvm
toolchain. This could have been prevented with running llvm::sort and
EXPENSIVE_CHECKS. Unfortunately, no sanitizer yelled.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116037

Files:
  clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp


Index: clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
===
--- clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
+++ clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
@@ -8,8 +8,9 @@
 
 #include "SymbolIndexManager.h"
 #include "find-all-symbols/SymbolInfo.h"
-#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 
@@ -47,7 +48,7 @@
 
 static void rank(std::vector ,
  llvm::StringRef FileName) {
-  llvm::DenseMap Score;
+  llvm::StringMap Score;
   for (const auto  : Symbols) {
 // Calculate a score from the similarity of the header the symbol is in
 // with the current file and the popularity of the symbol.
@@ -58,14 +59,14 @@
   }
   // Sort by the gathered scores. Use file name as a tie breaker so we can
   // deduplicate.
-  std::sort(Symbols.begin(), Symbols.end(),
-[&](const SymbolAndSignals , const SymbolAndSignals ) {
-  auto AS = Score[A.Symbol.getFilePath()];
-  auto BS = Score[B.Symbol.getFilePath()];
-  if (AS != BS)
-return AS > BS;
-  return A.Symbol.getFilePath() < B.Symbol.getFilePath();
-});
+  llvm::sort(Symbols.begin(), Symbols.end(),
+ [&](const SymbolAndSignals , const SymbolAndSignals ) {
+   auto AS = Score[A.Symbol.getFilePath()];
+   auto BS = Score[B.Symbol.getFilePath()];
+   if (AS != BS)
+ return AS > BS;
+   return A.Symbol.getFilePath() < B.Symbol.getFilePath();
+ });
 }
 
 std::vector


Index: clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
===
--- clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
+++ clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
@@ -8,8 +8,9 @@
 
 #include "SymbolIndexManager.h"
 #include "find-all-symbols/SymbolInfo.h"
-#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 
@@ -47,7 +48,7 @@
 
 static void rank(std::vector ,
  llvm::StringRef FileName) {
-  llvm::DenseMap Score;
+  llvm::StringMap Score;
   for (const auto  : Symbols) {
 // Calculate a score from the similarity of the header the symbol is in
 // with the current file and the popularity of the symbol.
@@ -58,14 +59,14 @@
   }
   // Sort by the gathered scores. Use file name as a tie breaker so we can
   // deduplicate.
-  std::sort(Symbols.begin(), Symbols.end(),
-[&](const SymbolAndSignals , const SymbolAndSignals ) {
-  auto AS = Score[A.Symbol.getFilePath()];
-  auto BS = Score[B.Symbol.getFilePath()];
-  if (AS != BS)
-return AS > BS;
-  return A.Symbol.getFilePath() < B.Symbol.getFilePath();
-});
+  llvm::sort(Symbols.begin(), Symbols.end(),
+ [&](const SymbolAndSignals , const SymbolAndSignals ) {
+   auto AS = Score[A.Symbol.getFilePath()];
+   auto BS = Score[B.Symbol.getFilePath()];
+   if (AS != BS)
+ return AS > BS;
+   return A.Symbol.getFilePath() < B.Symbol.getFilePath();
+ });
 }
 
 std::vector
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

Failure looks unrelated, please submit on your behalf as I don't have commit 
rights


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 345568.
danlark added a comment.

- Address comment from Richard


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
  clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
  clang/test/Parser/cxx2a-init-statement.cpp

Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,42 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: @_Z1fv(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND:%.*]]
+// CHECK:   for.cond:
+// CHECK-NEXT:[[TMP0:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[CMP:%.*]] = icmp slt i32 [[TMP0]], 2
+// CHECK-NEXT:[[TMP1:%.*]] = zext i1 [[CMP]] to i64
+// CHECK-NEXT:[[COND:%.*]] = select i1 [[CMP]], i32 1, i32 0
+// CHECK-NEXT:store i32 [[COND]], i32* [[X]], align 4
+// CHECK-NEXT:[[TMP2:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:[[TOBOOL:%.*]] = icmp ne i32 [[TMP2]], 0
+// CHECK-NEXT:br i1 [[TOBOOL]], label [[FOR_BODY:%.*]], label [[FOR_END:%.*]]
+// CHECK:   for.body:
+// CHECK-NEXT:[[TMP3:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:store i32 [[TMP3]], i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN:%.*]]
+// CHECK:   for.inc:
+// CHECK-NEXT:[[TMP4:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP4]], 1
+// CHECK-NEXT:store i32 [[INC]], i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND]], !llvm.loop [[LOOP2:![0-9]+]]
+// CHECK:   for.end:
+// CHECK-NEXT:store i32 0, i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN]]
+// CHECK:   return:
+// CHECK-NEXT:[[TMP5:%.*]] = load i32, i32* [[RETVAL]], align 4
+// CHECK-NEXT:ret i32 [[TMP5]]
+//
+int f() {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2033,6 +2033,8 @@
 DeclGroupPtrTy DG = ParseSimpleDeclaration(DeclaratorContext::ForInit,
DeclEnd, attrs, false, FRI);
 FRI->LoopVar = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
+assert((FRI->ColonLoc.isValid() || !DG) &&
+   "cannot find for range declaration");
 return Sema::ConditionResult();
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D102502#2759900 , @rsmith wrote:

> Thanks, nice catch!
>
> Can we also add an assert when parsing a `for` statement that we actually 
> find a range if the tentative parse said we were expecting one?

Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 345561.
danlark added a comment.

- Add assert of finding a for range declaration


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseExprCXX.cpp
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
  clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
  clang/test/Parser/cxx2a-init-statement.cpp

Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,42 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+// CHECK-LABEL: @_Z1fv(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND:%.*]]
+// CHECK:   for.cond:
+// CHECK-NEXT:[[TMP0:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[CMP:%.*]] = icmp slt i32 [[TMP0]], 2
+// CHECK-NEXT:[[TMP1:%.*]] = zext i1 [[CMP]] to i64
+// CHECK-NEXT:[[COND:%.*]] = select i1 [[CMP]], i32 1, i32 0
+// CHECK-NEXT:store i32 [[COND]], i32* [[X]], align 4
+// CHECK-NEXT:[[TMP2:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:[[TOBOOL:%.*]] = icmp ne i32 [[TMP2]], 0
+// CHECK-NEXT:br i1 [[TOBOOL]], label [[FOR_BODY:%.*]], label [[FOR_END:%.*]]
+// CHECK:   for.body:
+// CHECK-NEXT:[[TMP3:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:store i32 [[TMP3]], i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN:%.*]]
+// CHECK:   for.inc:
+// CHECK-NEXT:[[TMP4:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP4]], 1
+// CHECK-NEXT:store i32 [[INC]], i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND]], !llvm.loop [[LOOP2:![0-9]+]]
+// CHECK:   for.end:
+// CHECK-NEXT:store i32 0, i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN]]
+// CHECK:   return:
+// CHECK-NEXT:[[TMP5:%.*]] = load i32, i32* [[RETVAL]], align 4
+// CHECK-NEXT:ret i32 [[TMP5]]
+//
+int f() {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))
Index: clang/lib/Parse/ParseExprCXX.cpp
===
--- clang/lib/Parse/ParseExprCXX.cpp
+++ clang/lib/Parse/ParseExprCXX.cpp
@@ -2033,6 +2033,7 @@
 DeclGroupPtrTy DG = ParseSimpleDeclaration(DeclaratorContext::ForInit,
DeclEnd, attrs, false, FRI);
 FRI->LoopVar = Actions.ActOnDeclStmt(DG, DeclStart, Tok.getLocation());
+assert(FRI->ColonLoc.isValid() && "cannot find for range declaration");
 return Sema::ConditionResult();
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark added inline comments.



Comment at: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp:1
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | 
FileCheck %s
+

rsmith wrote:
> We don't use `-O3` tests for this kind of thing, to avoid depending on the 
> behaviour of the optimizer; instead you should test that the (unoptimized) IR 
> generated by clang is correct.
Done.



Comment at: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp:5
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {

rsmith wrote:
> This ast-print output looks wrong; I assume that's an unrelated bug?
Yes, I think so, in a buggy version `x` as a second parameter is missing so at 
least it tests correctly


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in the second for loop statement

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 345536.
danlark marked 3 inline comments as done.
danlark added a comment.

- Add codegen and AST tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
  clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
  clang/test/Parser/cxx2a-init-statement.cpp


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,42 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o - | 
FileCheck %s
+
+// CHECK-LABEL: @_Z1fv(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[RETVAL:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[I:%.*]] = alloca i32, align 4
+// CHECK-NEXT:[[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND:%.*]]
+// CHECK:   for.cond:
+// CHECK-NEXT:[[TMP0:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[CMP:%.*]] = icmp slt i32 [[TMP0]], 2
+// CHECK-NEXT:[[TMP1:%.*]] = zext i1 [[CMP]] to i64
+// CHECK-NEXT:[[COND:%.*]] = select i1 [[CMP]], i32 1, i32 0
+// CHECK-NEXT:store i32 [[COND]], i32* [[X]], align 4
+// CHECK-NEXT:[[TMP2:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:[[TOBOOL:%.*]] = icmp ne i32 [[TMP2]], 0
+// CHECK-NEXT:br i1 [[TOBOOL]], label [[FOR_BODY:%.*]], label 
[[FOR_END:%.*]]
+// CHECK:   for.body:
+// CHECK-NEXT:[[TMP3:%.*]] = load i32, i32* [[X]], align 4
+// CHECK-NEXT:store i32 [[TMP3]], i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN:%.*]]
+// CHECK:   for.inc:
+// CHECK-NEXT:[[TMP4:%.*]] = load i32, i32* [[I]], align 4
+// CHECK-NEXT:[[INC:%.*]] = add nsw i32 [[TMP4]], 1
+// CHECK-NEXT:store i32 [[INC]], i32* [[I]], align 4
+// CHECK-NEXT:br label [[FOR_COND]], !llvm.loop [[LOOP2:![0-9]+]]
+// CHECK:   for.end:
+// CHECK-NEXT:store i32 0, i32* [[RETVAL]], align 4
+// CHECK-NEXT:br label [[RETURN]]
+// CHECK:   return:
+// CHECK-NEXT:[[TMP5:%.*]] = load i32, i32* [[RETVAL]], align 4
+// CHECK-NEXT:ret i32 [[TMP5]]
+//
+int f() {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+  return 0;
+}
+
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-pch -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; 

[PATCH] D102502: [clang] Fix ternary operator in second for loop argument

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

Done, please, take a look


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in second for loop argument

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 345460.
danlark added a comment.

- Add codegen and AST tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
  clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
  clang/test/Parser/cxx2a-init-statement.cpp


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-pch -std=c++2a -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
+
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | 
FileCheck %s
+
+// CHECK-LABEL: define{{.*}} i32 @_Z1fv()
+int f() {
+  // CHECK: ret i32 1
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/PCH/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -emit-pch -std=c++2a -o %t %s
+// RUN: %clang_cc1 -x ast -ast-print %t | FileCheck %s
+
+int f() {
+  // CHECK: for (int i = 0; x; i++) {
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
+
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define{{.*}} i32 @_Z1fv()
+int f() {
+  // CHECK: ret i32 1
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in second for loop argument

[Danila Kutenin via Phabricator via cfe-commits]

danlark updated this revision to Diff 345458.
danlark added a comment.

- Add codegen test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
  clang/test/Parser/cxx2a-init-statement.cpp


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | 
FileCheck %s
+
+// CHECK-LABEL: define{{.*}} i32 @_Z1fv()
+int f() {
+  // CHECK: ret i32 1
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/for-loop-init-ternary-operator-statement.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define{{.*}} i32 @_Z1fv()
+int f() {
+  // CHECK: ret i32 1
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {
+return x;
+  }
+}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in second for loop argument

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D102502#2759776 , @lebedev.ri 
wrote:

> This should have codegen, and maybe AST, tests.

Will add shortly




Comment at: clang/test/Parser/cxx2a-init-statement.cpp:18
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+

lebedev.ri wrote:
> This isn't cxx2a-specific isn't it?
> Isn't this valid even in C99?
clang even in C++11 versions go to this branch and produce warning that this 
feature is c++2a, so it fails currently for everything but the culprit was 
https://github.com/llvm/llvm-project/commit/8baa50013c86c34a58d8327c5d1a043898b86398


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102502/new/

https://reviews.llvm.org/D102502

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102502: [clang] Fix ternary operator in second for loop argument

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
danlark added a reviewer: rsmith.
danlark requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix ternary operator in for loop argument, it was by mistake not set as 
CanBeForRangeDecl and led to incorrect codegen. It fixes 
https://bugs.llvm.org/show_bug.cgi?id=50038. I don't have commit rights. Danila 
Kutenin. kutdan...@yandex.ru


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102502

Files:
  clang/lib/Parse/ParseTentative.cpp
  clang/test/Parser/cxx2a-init-statement.cpp


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))


Index: clang/test/Parser/cxx2a-init-statement.cpp
===
--- clang/test/Parser/cxx2a-init-statement.cpp
+++ clang/test/Parser/cxx2a-init-statement.cpp
@@ -15,6 +15,8 @@
   int A<0>::*arr2[3];
   for (int n = 5; int A::*x : arr2) {}
 
+  for (int i = 0; int x = i < 2 ? 1 : 0; i++) {}
+
   F (*arr3[3])(int);
   for (int n = 5; F (*p)(int n) : arr3) {}
   for (int n = 5; F (*p)(int (n)) : arr3) {}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -353,8 +353,8 @@
   if (CanBeForRangeDecl) {
 // Skip until we hit a ')', ';', or a ':' with no matching '?'.
 // The final case is a for range declaration, the rest are not.
+unsigned QuestionColonDepth = 0;
 while (true) {
-  unsigned QuestionColonDepth = 0;
   P.SkipUntil({tok::r_paren, tok::semi, tok::question, tok::colon},
   StopBeforeMatch);
   if (P.Tok.is(tok::question))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97000: [clang] Increase the bitness of data length in ASTDeclContextNameLookup

[Danila Kutenin via Phabricator via cfe-commits]

danlark abandoned this revision.
danlark added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97000/new/

https://reviews.llvm.org/D97000

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97000: [clang] Increase the bitness of data length in ASTDeclContextNameLookup

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
danlark added a reviewer: rsmith.
danlark requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We faced the assert of too many declarations for serialized lookup for a very 
generated C++ target. We
think moving the type to uint32_t helps us. As I am not sure in the fix, please 
validate the correctness.

I don't have commit rights. Email: dani...@google.com. Name: Danila Kutenin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97000

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3599,9 +3599,7 @@
 
 // 4 bytes for each DeclID.
 unsigned DataLen = 4 * (Lookup.second - Lookup.first);
-assert(uint16_t(DataLen) == DataLen &&
-   "too many decls for serialized lookup result");
-LE.write(DataLen);
+LE.write(DataLen);
 
 return std::make_pair(KeyLen, DataLen);
   }
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1089,7 +1089,7 @@
   using namespace llvm::support;
 
   unsigned KeyLen = endian::readNext(d);
-  unsigned DataLen = endian::readNext(d);
+  unsigned DataLen = endian::readNext(d);
   return std::make_pair(KeyLen, DataLen);
 }
 


Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3599,9 +3599,7 @@
 
 // 4 bytes for each DeclID.
 unsigned DataLen = 4 * (Lookup.second - Lookup.first);
-assert(uint16_t(DataLen) == DataLen &&
-   "too many decls for serialized lookup result");
-LE.write(DataLen);
+LE.write(DataLen);
 
 return std::make_pair(KeyLen, DataLen);
   }
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -1089,7 +1089,7 @@
   using namespace llvm::support;
 
   unsigned KeyLen = endian::readNext(d);
-  unsigned DataLen = endian::readNext(d);
+  unsigned DataLen = endian::readNext(d);
   return std::make_pair(KeyLen, DataLen);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Danila Kutenin via Phabricator via cfe-commits]

danlark marked an inline comment as done.
danlark added inline comments.



Comment at: libcxx/trunk/include/__split_buffer:201
 __alloc_rr& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do

hiraditya wrote:
> danlark wrote:
> > lichray wrote:
> > > mclow.lists wrote:
> > > > I have been asked specifically by the optimizer folks to NOT do things 
> > > > like this in libc++, but rather to file bugs against the optimizer.
> > > > 
> > > > And I have done so for this exact case:  
> > > > https://bugs.llvm.org/show_bug.cgi?id=35637
> > > From the thread I didn't see that the compiler side asked you not to do 
> > > so.
> > > 
> > > And I disagree with the view.  libc++ shouldn't *wait* for compilers, 
> > > because we don't dictate users' compiler choices.  This change doesn't 
> > > make libc++ worse to coming compilers, and makes libc++ better on 
> > > existing compilers, so what benefit we get by not approving this?
> > So, what is the status? Are we waiting for the compiler code-gen fix?
> > 
> > At Yandex we are using patched version like half a year or more.
> > 
> > https://github.com/catboost/catboost/blob/master/contrib/libs/cxxsupp/libcxx/include/vector#L995
> It would be great to get this patch in. Waiting for compiler for this 
> optimization seems overkill.
It was separately submitted by the libcxx mainterner in July 2019 --  
https://reviews.llvm.org/rL367183


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44823/new/

https://reviews.llvm.org/D44823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D44823#1375590 , @mclow.lists wrote:

> I just tried this (on Compiler Explorer) using LLVM 7, and the code for my 
> original test in https://bugs.llvm.org/show_bug.cgi?id=35637 is now optimal.
>  Looking briefly at your test case, it seems to be fixed now too. Can you 
> confirm or disprove, please?


I don't see that it was fixed, f2 is clearing byte still

https://gcc.godbolt.org/z/kYuXJQ


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44823/new/

https://reviews.llvm.org/D44823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Danila Kutenin via Phabricator via cfe-commits]

danlark added inline comments.
Herald added a subscriber: ldionne.



Comment at: libcxx/trunk/include/__split_buffer:201
 __alloc_rr& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do

lichray wrote:
> mclow.lists wrote:
> > I have been asked specifically by the optimizer folks to NOT do things like 
> > this in libc++, but rather to file bugs against the optimizer.
> > 
> > And I have done so for this exact case:  
> > https://bugs.llvm.org/show_bug.cgi?id=35637
> From the thread I didn't see that the compiler side asked you not to do so.
> 
> And I disagree with the view.  libc++ shouldn't *wait* for compilers, because 
> we don't dictate users' compiler choices.  This change doesn't make libc++ 
> worse to coming compilers, and makes libc++ better on existing compilers, so 
> what benefit we get by not approving this?
So, what is the status? Are we waiting for the compiler code-gen fix?

At Yandex we are using patched version like half a year or more.

https://github.com/catboost/catboost/blob/master/contrib/libs/cxxsupp/libcxx/include/vector#L995


Repository:
  rCXX libc++

https://reviews.llvm.org/D44823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44823: [libcxx] Improving std::vector and std::deque perfomance

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
danlark added reviewers: EricWF, mclow.lists.
Herald added subscribers: cfe-commits, christof.

Consider the following code.

  #include 
  #include 
  
  class TestClass {
  public:
  TestClass(size_t size)
  : Data(size)
  {
  }
  private:
  std::vector Data;
  };
  
  int main(void) {
  std::unique_ptr test;
  for (int i = 0; i < 10; ++i)
  test.reset(new TestClass(0x1));
  return 0;
  }

For clang 5.0.1 it works for 14sec on my laptop. If you replace `char` by 
`short` it becomes 35 times faster(wow). The main difference in the generated 
code that for `char` no `memset` is called inside `__construct_at_end` function.

By manipulating a local variable in the loop, this lets it be fully optimized 
away.

Prior to this change, this would be generated (on x86-64):

  51,79c58,66
  <   movq  %rax, 8(%rbx)
  <   movq  %rax, (%rbx)
  <   movq  %rax, %rcx
  <   addq  $65536, %rcx# imm = 0x1
  <   movq  %rcx, 16(%rbx)
  <   movq  $-65536, %rcx   # imm = 0x
  <   .align  16, 0x90
  < .LBB0_4:#   Parent Loop BB0_1 Depth=1
  < # =>  This Inner Loop Header: 
Depth=2
  <   movb  $0, (%rax)
  <   movq  8(%rbx), %rax
  <   leaq  1(%rax), %rdx
  <   movq  %rdx, 8(%rbx)
  <   movb  $0, 1(%rax)
  <   movq  8(%rbx), %rax
  <   leaq  1(%rax), %rdx
  <   movq  %rdx, 8(%rbx)
  <   movb  $0, 1(%rax)
  <   movq  8(%rbx), %rax
  <   leaq  1(%rax), %rdx
  <   movq  %rdx, 8(%rbx)
  <   movb  $0, 1(%rax)
  <   movq  8(%rbx), %rax
  <   incq  %rax
  <   movq  %rax, 8(%rbx)
  <   addq  $4, %rcx
  <   jne  .LBB0_4
  < # BB#5: # %_ZN9TestClassC2Em.exit
  < #   in Loop: Header=BB0_1 Depth=1
  ---
  >   movq  %rax, (%r12)
  >   movq  %rax, %rbx
  >   addq  $65536, %rbx# imm = 0x1
  >   movq  %rbx, 16(%r12)
  >   xorl  %esi, %esi
  >   movl  $65536, %edx# imm = 0x1
  >   movq  %rax, %rdi
  >   callq  memset
  >   movq  %rbx, 8(%r12)
  81,82c68,69


Repository:
  rCXX libc++

https://reviews.llvm.org/D44823

Files:
  libcxx/trunk/include/__split_buffer
  libcxx/trunk/include/vector


Index: libcxx/trunk/include/vector
===
--- libcxx/trunk/include/vector
+++ libcxx/trunk/include/vector
@@ -984,11 +984,13 @@
 vector<_Tp, _Allocator>::__construct_at_end(size_type __n)
 {
 allocator_type& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do
 {
 __RAII_IncreaseAnnotator __annotator(*this);
-__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(this->__end_));
-++this->__end_;
+__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(__to_be_end));
+++__to_be_end;
+this->__end_ = __to_be_end;
 --__n;
 __annotator.__done();
 } while (__n > 0);
@@ -1006,11 +1008,13 @@
 vector<_Tp, _Allocator>::__construct_at_end(size_type __n, const_reference __x)
 {
 allocator_type& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do
 {
 __RAII_IncreaseAnnotator __annotator(*this);
-__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(this->__end_), 
__x);
-++this->__end_;
+__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(__to_be_end), 
__x);
+++__to_be_end;
+this->__end_ = __to_be_end;
 --__n;
 __annotator.__done();
 } while (__n > 0);
Index: libcxx/trunk/include/__split_buffer
===
--- libcxx/trunk/include/__split_buffer
+++ libcxx/trunk/include/__split_buffer
@@ -198,10 +198,12 @@
 __split_buffer<_Tp, _Allocator>::__construct_at_end(size_type __n)
 {
 __alloc_rr& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do
 {
-__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(this->__end_));
-++this->__end_;
+__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(__to_be_end));
+++__to_be_end;
+this->__end_ = __to_be_end;
 --__n;
 } while (__n > 0);
 }
@@ -217,10 +219,12 @@
 __split_buffer<_Tp, _Allocator>::__construct_at_end(size_type __n, 
const_reference __x)
 {
 __alloc_rr& __a = this->__alloc();
+pointer __to_be_end = this->__end_;
 do
 {
-__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(this->__end_), 
__x);
-++this->__end_;
+__alloc_traits::construct(__a, _VSTD::__to_raw_pointer(__to_be_end), 
__x);
+++__to_be_end;
+this->__end_ = __to_be_end;
 --__n;
 } while (__n > 0);
 }


Index: libcxx/trunk/include/vector
===
--- libcxx/trunk/include/vector
+++ libcxx/trunk/include/vector
@@ -984,11 +984,13 @@
 vector<_Tp,