subject:"\[PATCH\] D71913\: \[LTO\/WPD\] Enable aggressive WPD under LTO option"

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-07-14 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson marked an inline comment as done.
tejohnson added inline comments.
Herald added a reviewer: MaskRay.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> pcc wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > tejohnson wrote:
> > > > > evgeny777 wrote:
> > > > > > tejohnson wrote:
> > > > > > > evgeny777 wrote:
> > > > > > > > tejohnson wrote:
> > > > > > > > > evgeny777 wrote:
> > > > > > > > > > What caused this and other changes in this file?
> > > > > > > > > Because we now will insert type tests for non-hidden vtables. 
> > > > > > > > > This is enabled by the changes to LTO to interpret these 
> > > > > > > > > based on the vcall_visibility metadata.
> > > > > > > > The results of this test case
> > > > > > > > ```
> > > > > > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > > > > > > -fms-extensions -fwhole-program-vtables 
> > > > > > > > -flto-visibility-public-std -emit-llvm -o - %s | FileCheck 
> > > > > > > > --check-prefix=MS --check-prefix=MS-NOSTD %s
> > > > > > > > ```
> > > > > > > > look not correct to me. I think you shouldn't generate type 
> > > > > > > > tests for standard library classes with  
> > > > > > > > `-flto-visibility-public-std`. Currently if this flag is given, 
> > > > > > > > clang doesn't do this either even with `-fvisibility=hidden`
> > > > > > > The associated vtables would get the vcall_visibility public 
> > > > > > > metadata, so the type tests themselves aren't problematic. I tend 
> > > > > > > to think that an application using such options should simply 
> > > > > > > stick with -fvisibility=hidden to get WPD and not use the new LTO 
> > > > > > > option to convert all public vcall_visibility metadata to hidden.
> > > > > > > The associated vtables would get the vcall_visibility public 
> > > > > > > metadata, so the type tests themselves aren't problematic. I tend 
> > > > > > > to think that an application using such options should simply 
> > > > > > > stick with -fvisibility=hidden to get WPD and not use the new LTO 
> > > > > > > option to convert all public vcall_visibility metadata to hidden.
> > > > > > 
> > > > > > I see two issues here:
> > > > > > 1) It's not always good option to force hidden visibility for 
> > > > > > everything. For instance I work on proprietary platform which 
> > > > > > demands public visibility for certain symbols in order for dynamic 
> > > > > > loader to work properly. In this context your patch does a great 
> > > > > > job.
> > > > > > 
> > > > > > 2) Standard library is almost never LTOed so in general we can't 
> > > > > > narrow std::* vtables visibility to linkage unit
> > > > > > 
> > > > > > Is there anything which prevents from implementing the same 
> > > > > > functionality with new -lto-whole-program-visibility option (i.e 
> > > > > > without forcing hidden visibility)? In other words the following 
> > > > > > looks good to me:
> > > > > > 
> > > > > > ```
> > > > > > # Compile with lto/devirtualization support
> > > > > > clang -flto=thin -flto-visibility-public-std 
> > > > > > -fwhole-program-vtables -c *.cpp
> > > > > > 
> > > > > > # Link: everything is devirtualized except standard library classes 
> > > > > > virtual methods
> > > > > > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > > > > > ```
> > > > > Ok, thanks for the info. I will go ahead and change the code to not 
> > > > > insert the type tests in this case to support this usage.
> > > > > 
> > > > > Ultimately, I would like to decouple the existence of the type tests 
> > > > > from visibility implications. I'm working on another change to delay 
> > > > > lowering/removal of type tests until after indirect call promotion, 
> > > > > so we can use them in other cases (streamlined indirect call 
> > > > > promotion checks against the vtable instead of the function pointers, 
> > > > > also useful if we want to implement speculative devirtualization 
> > > > > based on WPD info). In those cases we need the type tests, either to 
> > > > > locate information in the summary, or to get the address point offset 
> > > > > for a vtable address compare. In that case it would be helpful to 
> > > > > have the type tests in this type of code as well. So we'll need 
> > > > > another way to communicate down to WPD that they should never be 
> > > > > devirtualized. But I don't think it makes sense to design this 
> > > > > support until there is a concrete use case and need. In the meantime 
> > > > > I will change the code to be conservative and not insert the type 
> > > > > tests in this case.
> > > > Note that `-flto-visibility-public-std` is a cc1-only option and only 
> > > > used on Windows, and further that `-lto-whole-program-visibility` as 
> > > > implemented doesn't really make sense on

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-03-17 Thread Teresa Johnson via Phabricator via cfe-commits

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



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

pcc wrote:
> tejohnson wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > evgeny777 wrote:
> > > > > tejohnson wrote:
> > > > > > evgeny777 wrote:
> > > > > > > tejohnson wrote:
> > > > > > > > evgeny777 wrote:
> > > > > > > > > What caused this and other changes in this file?
> > > > > > > > Because we now will insert type tests for non-hidden vtables. 
> > > > > > > > This is enabled by the changes to LTO to interpret these based 
> > > > > > > > on the vcall_visibility metadata.
> > > > > > > The results of this test case
> > > > > > > ```
> > > > > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > > > > > -fms-extensions -fwhole-program-vtables 
> > > > > > > -flto-visibility-public-std -emit-llvm -o - %s | FileCheck 
> > > > > > > --check-prefix=MS --check-prefix=MS-NOSTD %s
> > > > > > > ```
> > > > > > > look not correct to me. I think you shouldn't generate type tests 
> > > > > > > for standard library classes with  `-flto-visibility-public-std`. 
> > > > > > > Currently if this flag is given, clang doesn't do this either 
> > > > > > > even with `-fvisibility=hidden`
> > > > > > The associated vtables would get the vcall_visibility public 
> > > > > > metadata, so the type tests themselves aren't problematic. I tend 
> > > > > > to think that an application using such options should simply stick 
> > > > > > with -fvisibility=hidden to get WPD and not use the new LTO option 
> > > > > > to convert all public vcall_visibility metadata to hidden.
> > > > > > The associated vtables would get the vcall_visibility public 
> > > > > > metadata, so the type tests themselves aren't problematic. I tend 
> > > > > > to think that an application using such options should simply stick 
> > > > > > with -fvisibility=hidden to get WPD and not use the new LTO option 
> > > > > > to convert all public vcall_visibility metadata to hidden.
> > > > > 
> > > > > I see two issues here:
> > > > > 1) It's not always good option to force hidden visibility for 
> > > > > everything. For instance I work on proprietary platform which demands 
> > > > > public visibility for certain symbols in order for dynamic loader to 
> > > > > work properly. In this context your patch does a great job.
> > > > > 
> > > > > 2) Standard library is almost never LTOed so in general we can't 
> > > > > narrow std::* vtables visibility to linkage unit
> > > > > 
> > > > > Is there anything which prevents from implementing the same 
> > > > > functionality with new -lto-whole-program-visibility option (i.e 
> > > > > without forcing hidden visibility)? In other words the following 
> > > > > looks good to me:
> > > > > 
> > > > > ```
> > > > > # Compile with lto/devirtualization support
> > > > > clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables 
> > > > > -c *.cpp
> > > > > 
> > > > > # Link: everything is devirtualized except standard library classes 
> > > > > virtual methods
> > > > > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > > > > ```
> > > > Ok, thanks for the info. I will go ahead and change the code to not 
> > > > insert the type tests in this case to support this usage.
> > > > 
> > > > Ultimately, I would like to decouple the existence of the type tests 
> > > > from visibility implications. I'm working on another change to delay 
> > > > lowering/removal of type tests until after indirect call promotion, so 
> > > > we can use them in other cases (streamlined indirect call promotion 
> > > > checks against the vtable instead of the function pointers, also useful 
> > > > if we want to implement speculative devirtualization based on WPD 
> > > > info). In those cases we need the type tests, either to locate 
> > > > information in the summary, or to get the address point offset for a 
> > > > vtable address compare. In that case it would be helpful to have the 
> > > > type tests in this type of code as well. So we'll need another way to 
> > > > communicate down to WPD that they should never be devirtualized. But I 
> > > > don't think it makes sense to design this support until there is a 
> > > > concrete use case and need. In the meantime I will change the code to 
> > > > be conservative and not insert the type tests in this case.
> > > Note that `-flto-visibility-public-std` is a cc1-only option and only 
> > > used on Windows, and further that `-lto-whole-program-visibility` as 
> > > implemented doesn't really make sense on Windows because the classes with 
> > > public visibility are going to be marked dllimport/dllexport/uuid (COM), 
> > > and `-lto-whole-program-visibility` corresponds to flags such as 
> > > `--version-script` or the absence of

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-03-05 Thread Peter Collingbourne via Phabricator via cfe-commits

pcc added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> pcc wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > tejohnson wrote:
> > > > > evgeny777 wrote:
> > > > > > tejohnson wrote:
> > > > > > > evgeny777 wrote:
> > > > > > > > What caused this and other changes in this file?
> > > > > > > Because we now will insert type tests for non-hidden vtables. 
> > > > > > > This is enabled by the changes to LTO to interpret these based on 
> > > > > > > the vcall_visibility metadata.
> > > > > > The results of this test case
> > > > > > ```
> > > > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > > > > -fms-extensions -fwhole-program-vtables -flto-visibility-public-std 
> > > > > > -emit-llvm -o - %s | FileCheck --check-prefix=MS 
> > > > > > --check-prefix=MS-NOSTD %s
> > > > > > ```
> > > > > > look not correct to me. I think you shouldn't generate type tests 
> > > > > > for standard library classes with  `-flto-visibility-public-std`. 
> > > > > > Currently if this flag is given, clang doesn't do this either even 
> > > > > > with `-fvisibility=hidden`
> > > > > The associated vtables would get the vcall_visibility public 
> > > > > metadata, so the type tests themselves aren't problematic. I tend to 
> > > > > think that an application using such options should simply stick with 
> > > > > -fvisibility=hidden to get WPD and not use the new LTO option to 
> > > > > convert all public vcall_visibility metadata to hidden.
> > > > > The associated vtables would get the vcall_visibility public 
> > > > > metadata, so the type tests themselves aren't problematic. I tend to 
> > > > > think that an application using such options should simply stick with 
> > > > > -fvisibility=hidden to get WPD and not use the new LTO option to 
> > > > > convert all public vcall_visibility metadata to hidden.
> > > > 
> > > > I see two issues here:
> > > > 1) It's not always good option to force hidden visibility for 
> > > > everything. For instance I work on proprietary platform which demands 
> > > > public visibility for certain symbols in order for dynamic loader to 
> > > > work properly. In this context your patch does a great job.
> > > > 
> > > > 2) Standard library is almost never LTOed so in general we can't narrow 
> > > > std::* vtables visibility to linkage unit
> > > > 
> > > > Is there anything which prevents from implementing the same 
> > > > functionality with new -lto-whole-program-visibility option (i.e 
> > > > without forcing hidden visibility)? In other words the following looks 
> > > > good to me:
> > > > 
> > > > ```
> > > > # Compile with lto/devirtualization support
> > > > clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c 
> > > > *.cpp
> > > > 
> > > > # Link: everything is devirtualized except standard library classes 
> > > > virtual methods
> > > > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > > > ```
> > > Ok, thanks for the info. I will go ahead and change the code to not 
> > > insert the type tests in this case to support this usage.
> > > 
> > > Ultimately, I would like to decouple the existence of the type tests from 
> > > visibility implications. I'm working on another change to delay 
> > > lowering/removal of type tests until after indirect call promotion, so we 
> > > can use them in other cases (streamlined indirect call promotion checks 
> > > against the vtable instead of the function pointers, also useful if we 
> > > want to implement speculative devirtualization based on WPD info). In 
> > > those cases we need the type tests, either to locate information in the 
> > > summary, or to get the address point offset for a vtable address compare. 
> > > In that case it would be helpful to have the type tests in this type of 
> > > code as well. So we'll need another way to communicate down to WPD that 
> > > they should never be devirtualized. But I don't think it makes sense to 
> > > design this support until there is a concrete use case and need. In the 
> > > meantime I will change the code to be conservative and not insert the 
> > > type tests in this case.
> > Note that `-flto-visibility-public-std` is a cc1-only option and only used 
> > on Windows, and further that `-lto-whole-program-visibility` as implemented 
> > doesn't really make sense on Windows because the classes with public 
> > visibility are going to be marked dllimport/dllexport/uuid (COM), and 
> > `-lto-whole-program-visibility` corresponds to flags such as 
> > `--version-script` or the absence of `-shared` in which the linker 
> > automatically relaxes the visibility of some symbols, while there is no 
> > such concept of relaxing symbol visibility on Windows.
> > 
> >  I would be inclined to remove this support and

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-03-04 Thread Teresa Johnson via Phabricator via cfe-commits

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



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

pcc wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > tejohnson wrote:
> > > > evgeny777 wrote:
> > > > > tejohnson wrote:
> > > > > > evgeny777 wrote:
> > > > > > > What caused this and other changes in this file?
> > > > > > Because we now will insert type tests for non-hidden vtables. This 
> > > > > > is enabled by the changes to LTO to interpret these based on the 
> > > > > > vcall_visibility metadata.
> > > > > The results of this test case
> > > > > ```
> > > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > > > -fms-extensions -fwhole-program-vtables -flto-visibility-public-std 
> > > > > -emit-llvm -o - %s | FileCheck --check-prefix=MS 
> > > > > --check-prefix=MS-NOSTD %s
> > > > > ```
> > > > > look not correct to me. I think you shouldn't generate type tests for 
> > > > > standard library classes with  `-flto-visibility-public-std`. 
> > > > > Currently if this flag is given, clang doesn't do this either even 
> > > > > with `-fvisibility=hidden`
> > > > The associated vtables would get the vcall_visibility public metadata, 
> > > > so the type tests themselves aren't problematic. I tend to think that 
> > > > an application using such options should simply stick with 
> > > > -fvisibility=hidden to get WPD and not use the new LTO option to 
> > > > convert all public vcall_visibility metadata to hidden.
> > > > The associated vtables would get the vcall_visibility public metadata, 
> > > > so the type tests themselves aren't problematic. I tend to think that 
> > > > an application using such options should simply stick with 
> > > > -fvisibility=hidden to get WPD and not use the new LTO option to 
> > > > convert all public vcall_visibility metadata to hidden.
> > > 
> > > I see two issues here:
> > > 1) It's not always good option to force hidden visibility for everything. 
> > > For instance I work on proprietary platform which demands public 
> > > visibility for certain symbols in order for dynamic loader to work 
> > > properly. In this context your patch does a great job.
> > > 
> > > 2) Standard library is almost never LTOed so in general we can't narrow 
> > > std::* vtables visibility to linkage unit
> > > 
> > > Is there anything which prevents from implementing the same functionality 
> > > with new -lto-whole-program-visibility option (i.e without forcing hidden 
> > > visibility)? In other words the following looks good to me:
> > > 
> > > ```
> > > # Compile with lto/devirtualization support
> > > clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c 
> > > *.cpp
> > > 
> > > # Link: everything is devirtualized except standard library classes 
> > > virtual methods
> > > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > > ```
> > Ok, thanks for the info. I will go ahead and change the code to not insert 
> > the type tests in this case to support this usage.
> > 
> > Ultimately, I would like to decouple the existence of the type tests from 
> > visibility implications. I'm working on another change to delay 
> > lowering/removal of type tests until after indirect call promotion, so we 
> > can use them in other cases (streamlined indirect call promotion checks 
> > against the vtable instead of the function pointers, also useful if we want 
> > to implement speculative devirtualization based on WPD info). In those 
> > cases we need the type tests, either to locate information in the summary, 
> > or to get the address point offset for a vtable address compare. In that 
> > case it would be helpful to have the type tests in this type of code as 
> > well. So we'll need another way to communicate down to WPD that they should 
> > never be devirtualized. But I don't think it makes sense to design this 
> > support until there is a concrete use case and need. In the meantime I will 
> > change the code to be conservative and not insert the type tests in this 
> > case.
> Note that `-flto-visibility-public-std` is a cc1-only option and only used on 
> Windows, and further that `-lto-whole-program-visibility` as implemented 
> doesn't really make sense on Windows because the classes with public 
> visibility are going to be marked dllimport/dllexport/uuid (COM), and 
> `-lto-whole-program-visibility` corresponds to flags such as 
> `--version-script` or the absence of `-shared` in which the linker 
> automatically relaxes the visibility of some symbols, while there is no such 
> concept of relaxing symbol visibility on Windows.
> 
>  I would be inclined to remove this support and either let the public 
> visibility automatically derive from the absence of 
> `-lto-whole-program-visibility` at link time in COFF links or

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-03-02 Thread Peter Collingbourne via Phabricator via cfe-commits

pcc added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > tejohnson wrote:
> > > > > evgeny777 wrote:
> > > > > > What caused this and other changes in this file?
> > > > > Because we now will insert type tests for non-hidden vtables. This is 
> > > > > enabled by the changes to LTO to interpret these based on the 
> > > > > vcall_visibility metadata.
> > > > The results of this test case
> > > > ```
> > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > > -fms-extensions -fwhole-program-vtables -flto-visibility-public-std 
> > > > -emit-llvm -o - %s | FileCheck --check-prefix=MS 
> > > > --check-prefix=MS-NOSTD %s
> > > > ```
> > > > look not correct to me. I think you shouldn't generate type tests for 
> > > > standard library classes with  `-flto-visibility-public-std`. Currently 
> > > > if this flag is given, clang doesn't do this either even with 
> > > > `-fvisibility=hidden`
> > > The associated vtables would get the vcall_visibility public metadata, so 
> > > the type tests themselves aren't problematic. I tend to think that an 
> > > application using such options should simply stick with 
> > > -fvisibility=hidden to get WPD and not use the new LTO option to convert 
> > > all public vcall_visibility metadata to hidden.
> > > The associated vtables would get the vcall_visibility public metadata, so 
> > > the type tests themselves aren't problematic. I tend to think that an 
> > > application using such options should simply stick with 
> > > -fvisibility=hidden to get WPD and not use the new LTO option to convert 
> > > all public vcall_visibility metadata to hidden.
> > 
> > I see two issues here:
> > 1) It's not always good option to force hidden visibility for everything. 
> > For instance I work on proprietary platform which demands public visibility 
> > for certain symbols in order for dynamic loader to work properly. In this 
> > context your patch does a great job.
> > 
> > 2) Standard library is almost never LTOed so in general we can't narrow 
> > std::* vtables visibility to linkage unit
> > 
> > Is there anything which prevents from implementing the same functionality 
> > with new -lto-whole-program-visibility option (i.e without forcing hidden 
> > visibility)? In other words the following looks good to me:
> > 
> > ```
> > # Compile with lto/devirtualization support
> > clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c 
> > *.cpp
> > 
> > # Link: everything is devirtualized except standard library classes virtual 
> > methods
> > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > ```
> Ok, thanks for the info. I will go ahead and change the code to not insert 
> the type tests in this case to support this usage.
> 
> Ultimately, I would like to decouple the existence of the type tests from 
> visibility implications. I'm working on another change to delay 
> lowering/removal of type tests until after indirect call promotion, so we can 
> use them in other cases (streamlined indirect call promotion checks against 
> the vtable instead of the function pointers, also useful if we want to 
> implement speculative devirtualization based on WPD info). In those cases we 
> need the type tests, either to locate information in the summary, or to get 
> the address point offset for a vtable address compare. In that case it would 
> be helpful to have the type tests in this type of code as well. So we'll need 
> another way to communicate down to WPD that they should never be 
> devirtualized. But I don't think it makes sense to design this support until 
> there is a concrete use case and need. In the meantime I will change the code 
> to be conservative and not insert the type tests in this case.
Note that `-flto-visibility-public-std` is a cc1-only option and only used on 
Windows, and further that `-lto-whole-program-visibility` as implemented 
doesn't really make sense on Windows because the classes with public visibility 
are going to be marked dllimport/dllexport/uuid (COM), and 
`-lto-whole-program-visibility` corresponds to flags such as `--version-script` 
or the absence of `-shared` in which the linker automatically relaxes the 
visibility of some symbols, while there is no such concept of relaxing symbol 
visibility on Windows.

 I would be inclined to remove this support and either let the public 
visibility automatically derive from the absence of 
`-lto-whole-program-visibility` at link time in COFF links or omit the IR 
needed to support `lto-whole-program-visibility` on Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-27 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

In D71913#1837872 , @tejohnson wrote:

> FYI I reverted this in rG90e630a95ecc 
>  due to 
> a cfi test failure in a windows sanitizer bot. Not sure what is happening, 
> I'll need to try to debug it somehow tomorrow.

This patch went back in a little while ago at 
2f63d549f1e1edd165392837aaa53f569f7fb88d 
, and 
looks like it will stick this time. The windows bot no longer fails with this 
patch after the enabling fix in D73418  went 
in before this at
af954e441a5170a75687699d91d85e0692929d43 
.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-23 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

FYI I reverted this in rG90e630a95ecc 
 due to a 
cfi test failure in a windows sanitizer bot. Not sure what is happening, I'll 
need to try to debug it somehow tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-23 Thread Teresa Johnson via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG59733525d37c: [LTO/WPD] Enable aggressive WPD under LTO 
option (authored by tejohnson).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/cfi-mfcall.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-23 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 accepted this revision.
evgeny777 added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-23 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 239890.
tejohnson added a comment.

Implement suggestion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/cfi-mfcall.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp
===
--- llvm/tools/gold/gold-plugin.cpp
+++

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-23 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added a comment.

Looks good so far. See remaining comment in D71907 





Comment at: clang/lib/CodeGen/CGVTables.cpp:1050
 
-  if (getCodeGenOpts().LTOVisibilityPublicStd) {
-const DeclContext *DC = RD;
-while (1) {
-  auto *D = cast(DC);
-  DC = DC->getParent();
-  if (isa(DC->getRedeclContext())) {
-if (auto *ND = dyn_cast(D))
-  if (const IdentifierInfo *II = ND->getIdentifier())
-if (II->isStr("std") || II->isStr("stdext"))
-  return false;
-break;
-  }
-}
-  }
+  if (HasLTOVisibilityPublicStd(RD))
+return false;

nit: return !HasLTOVisibilityPublicStd(RD)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-22 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 239646.
tejohnson added a comment.

Address remaining comment by blocking type test insertion for public std 
visibility


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/cfi-mfcall.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-22 Thread Teresa Johnson via Phabricator via cfe-commits

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



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

evgeny777 wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > tejohnson wrote:
> > > > evgeny777 wrote:
> > > > > What caused this and other changes in this file?
> > > > Because we now will insert type tests for non-hidden vtables. This is 
> > > > enabled by the changes to LTO to interpret these based on the 
> > > > vcall_visibility metadata.
> > > The results of this test case
> > > ```
> > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 
> > > -fms-extensions -fwhole-program-vtables -flto-visibility-public-std 
> > > -emit-llvm -o - %s | FileCheck --check-prefix=MS --check-prefix=MS-NOSTD 
> > > %s
> > > ```
> > > look not correct to me. I think you shouldn't generate type tests for 
> > > standard library classes with  `-flto-visibility-public-std`. Currently 
> > > if this flag is given, clang doesn't do this either even with 
> > > `-fvisibility=hidden`
> > The associated vtables would get the vcall_visibility public metadata, so 
> > the type tests themselves aren't problematic. I tend to think that an 
> > application using such options should simply stick with -fvisibility=hidden 
> > to get WPD and not use the new LTO option to convert all public 
> > vcall_visibility metadata to hidden.
> > The associated vtables would get the vcall_visibility public metadata, so 
> > the type tests themselves aren't problematic. I tend to think that an 
> > application using such options should simply stick with -fvisibility=hidden 
> > to get WPD and not use the new LTO option to convert all public 
> > vcall_visibility metadata to hidden.
> 
> I see two issues here:
> 1) It's not always good option to force hidden visibility for everything. For 
> instance I work on proprietary platform which demands public visibility for 
> certain symbols in order for dynamic loader to work properly. In this context 
> your patch does a great job.
> 
> 2) Standard library is almost never LTOed so in general we can't narrow 
> std::* vtables visibility to linkage unit
> 
> Is there anything which prevents from implementing the same functionality 
> with new -lto-whole-program-visibility option (i.e without forcing hidden 
> visibility)? In other words the following looks good to me:
> 
> ```
> # Compile with lto/devirtualization support
> clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp
> 
> # Link: everything is devirtualized except standard library classes virtual 
> methods
> clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> ```
Ok, thanks for the info. I will go ahead and change the code to not insert the 
type tests in this case to support this usage.

Ultimately, I would like to decouple the existence of the type tests from 
visibility implications. I'm working on another change to delay 
lowering/removal of type tests until after indirect call promotion, so we can 
use them in other cases (streamlined indirect call promotion checks against the 
vtable instead of the function pointers, also useful if we want to implement 
speculative devirtualization based on WPD info). In those cases we need the 
type tests, either to locate information in the summary, or to get the address 
point offset for a vtable address compare. In that case it would be helpful to 
have the type tests in this type of code as well. So we'll need another way to 
communicate down to WPD that they should never be devirtualized. But I don't 
think it makes sense to design this support until there is a concrete use case 
and need. In the meantime I will change the code to be conservative and not 
insert the type tests in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-22 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > What caused this and other changes in this file?
> > > Because we now will insert type tests for non-hidden vtables. This is 
> > > enabled by the changes to LTO to interpret these based on the 
> > > vcall_visibility metadata.
> > The results of this test case
> > ```
> > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions 
> > -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | 
> > FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
> > ```
> > look not correct to me. I think you shouldn't generate type tests for 
> > standard library classes with  `-flto-visibility-public-std`. Currently if 
> > this flag is given, clang doesn't do this either even with 
> > `-fvisibility=hidden`
> The associated vtables would get the vcall_visibility public metadata, so the 
> type tests themselves aren't problematic. I tend to think that an application 
> using such options should simply stick with -fvisibility=hidden to get WPD 
> and not use the new LTO option to convert all public vcall_visibility 
> metadata to hidden.
> The associated vtables would get the vcall_visibility public metadata, so the 
> type tests themselves aren't problematic. I tend to think that an application 
> using such options should simply stick with -fvisibility=hidden to get WPD 
> and not use the new LTO option to convert all public vcall_visibility 
> metadata to hidden.

I see two issues here:
1) It's not always good option to force hidden visibility for everything. For 
instance I work on proprietary platform which demands public visibility for 
certain symbols in order for dynamic loader to work properly. In this context 
your patch does a great job.

2) Standard library is almost never LTOed so in general we can't narrow std::* 
vtables visibility to linkage unit

Is there anything which prevents from implementing the same functionality with 
new -lto-whole-program-visibility option (i.e without forcing hidden 
visibility)? In other words the following looks good to me:

```
# Compile with lto/devirtualization support
clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp

# Link: everything is devirtualized except standard library classes virtual 
methods
clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-21 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson marked 5 inline comments as done.
tejohnson added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

evgeny777 wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > What caused this and other changes in this file?
> > Because we now will insert type tests for non-hidden vtables. This is 
> > enabled by the changes to LTO to interpret these based on the 
> > vcall_visibility metadata.
> The results of this test case
> ```
> %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions 
> -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | 
> FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
> ```
> look not correct to me. I think you shouldn't generate type tests for 
> standard library classes with  `-flto-visibility-public-std`. Currently if 
> this flag is given, clang doesn't do this either even with 
> `-fvisibility=hidden`
The associated vtables would get the vcall_visibility public metadata, so the 
type tests themselves aren't problematic. I tend to think that an application 
using such options should simply stick with -fvisibility=hidden to get WPD and 
not use the new LTO option to convert all public vcall_visibility metadata to 
hidden.



Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:7
+// as expected.
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux 
-fvisibility hidden -fwhole-program-vtables -emit-llvm-bc -o %t.o %s
+// RUN: llvm-dis %t.o -o - | FileCheck --check-prefix=TT %s

evgeny777 wrote:
> I think, we no longer need `-fvisibility hidden` here, do we?
Right, we aren't relying on the visibility for this test which is just ensuring 
the type tests are lowered properly in the distributed backends. I removed it.



Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+  // via the internal option. Must be done before WPD below.
+  updateVCallVisibilityInIndex(*Index);
+

evgeny777 wrote:
> evgeny777 wrote:
> > ditto
> updateVCallVisibilityInIndex(*Index, /* WholeProgramVisibilityEnabledInLTO */ 
> false) ?
Missed this one before (did the InModule version but not InIndex), fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-21 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 239428.
tejohnson marked 2 inline comments as done.
tejohnson added a comment.

Address comments and rebase. Also apply modified change to ItaniumCXXABI.cpp
and a change to an associated test (cfi-mfcall.cpp) here as discussed
in child revision D71907 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/cfi-mfcall.cpp
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-21 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added inline comments.



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

tejohnson wrote:
> evgeny777 wrote:
> > What caused this and other changes in this file?
> Because we now will insert type tests for non-hidden vtables. This is enabled 
> by the changes to LTO to interpret these based on the vcall_visibility 
> metadata.
The results of this test case
```
%clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions 
-fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | 
FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
```
look not correct to me. I think you shouldn't generate type tests for standard 
library classes with  `-flto-visibility-public-std`. Currently if this flag is 
given, clang doesn't do this either even with `-fvisibility=hidden`



Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:7
+// as expected.
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux 
-fvisibility hidden -fwhole-program-vtables -emit-llvm-bc -o %t.o %s
+// RUN: llvm-dis %t.o -o - | FileCheck --check-prefix=TT %s

I think, we no longer need `-fvisibility hidden` here, do we?



Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+  // via the internal option. Must be done before WPD below.
+  updateVCallVisibilityInIndex(*Index);
+

evgeny777 wrote:
> ditto
updateVCallVisibilityInIndex(*Index, /* WholeProgramVisibilityEnabledInLTO */ 
false) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-20 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 239214.
tejohnson marked 2 inline comments as done.
tejohnson added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp
===
--- llvm/tools/gold/gold-plugin.cpp
+++ llvm/tools/gold/gold-plugin.cpp
@@ -204,6 +204,8 @@
   static std::string dwo_dir;
   /// Statistics output

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-20 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson marked 4 inline comments as done.
tejohnson added inline comments.



Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

evgeny777 wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > Why do you need `-whole-program-visibility` here? Correct me if I'm 
> > > wrong, but AFAIK module scanning doesn't happen during import and GV 
> > > visibility should be taken from imported summary.
> > Before my patch, type tests were only inserted for vtables with hidden LTO 
> > visibility. Therefore, the very presence of type tests communicated the 
> > hidden visibility and enabled the WPD.
> > 
> > With this patch, to support enabling WPD aggressively at LTO time, we now 
> > insert type tests unconditionally under -fwhole-program-vtables. The 
> > vcall_visibility metadata tells LTO how to interpret them. And the new 
> > options allow changing those to hidden visibility to get the more 
> > aggressive WPD.
> > 
> > Because these legacy tests have type tests but no vcall_visibility 
> > metadata, we now will conservatively treat them as having public LTO 
> > visibility. This option is therefore required to convert the summarized 
> > (default public) vcall visibility into hidden to get the prior more 
> > aggressive behavior they are trying to test.
> > 
> > Note I could have instead changed the assembly here to add hidden 
> > vcall_visibility metadata everywhere. That seemed a little onerous so 
> > that's why I just added the option. I could add a comment to that effect if 
> > it would help?
> I think you can remove this option from this test (and probably others using 
> -wholeprogramdevirt-summary-action=import option), because visibility seems 
> to be not analyzed on import phase. I just did this btw and test still passes 
> flawlessly.
Good point, removed from here and a couple other similar tests. I added it a 
little overeagerly to address the failures I got originally.



Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

evgeny777 wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > Hm, looks like I don't fully understand this. I have following concerns:
> > > 
> > > 1) According to your changes every time I use `opt -wholeprogramdevirt` I 
> > > also have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` 
> > > flag become no-op without this additional flag? If so this looks counter 
> > > intuitive to me.
> > > 
> > > 2) When I use `opt -wholeprogramdevirt` default constructor of 
> > > `WholeProgramDevirt` class is called and `UseCommandLine` flag is set to 
> > > true. Can't I use this flag to effectively lower visibility in module 
> > > instead of playing with metadata?
> > > 
> > > ```
> > > if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
> > > !UseCommandLine)
> > >  return false;
> > > ```
> > > 
> > > According to your changes every time I use opt -wholeprogramdevirt I also 
> > > have to pass -whole-program-visibility. Has -wholeprogramdevirt flag 
> > > become no-op without this additional flag? If so this looks counter 
> > > intuitive to me.
> > 
> > No, it wouldn't be needed if the tests contained !vcall_visibility metadata 
> > indicating hidden LTO visibility (see my earlier comment response).
> > 
> > > When I use opt -wholeprogramdevirt default constructor of 
> > > WholeProgramDevirt class is called and UseCommandLine flag is set to 
> > > true. Can't I use this flag to effectively lower visibility in module 
> > > instead of playing with metadata?
> > 
> > I could do that. What it would mean though is that we would be unable to 
> > use opt for any future testing of vtables intended to have public vcall 
> > visibility (either through a lack of that metadata, or explicit 
> > vcall_vsibility metadata indicating public). Which might be ok - in fact 
> > all my new testing of this behavior is via llvm-lto2 or the linkers. I 
> > suppose that would obviate this change as well as all the opt based test 
> > changes to pass the flag. Do you think that is better?
> > I could do that. What it would mean though is that we would be unable to 
> > use opt for any future testing of vtables intended to have public vcall 
> > visibility (either through a lack of that metadata, or

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-17 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added inline comments.



Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

tejohnson wrote:
> evgeny777 wrote:
> > Why do you need `-whole-program-visibility` here? Correct me if I'm wrong, 
> > but AFAIK module scanning doesn't happen during import and GV visibility 
> > should be taken from imported summary.
> Before my patch, type tests were only inserted for vtables with hidden LTO 
> visibility. Therefore, the very presence of type tests communicated the 
> hidden visibility and enabled the WPD.
> 
> With this patch, to support enabling WPD aggressively at LTO time, we now 
> insert type tests unconditionally under -fwhole-program-vtables. The 
> vcall_visibility metadata tells LTO how to interpret them. And the new 
> options allow changing those to hidden visibility to get the more aggressive 
> WPD.
> 
> Because these legacy tests have type tests but no vcall_visibility metadata, 
> we now will conservatively treat them as having public LTO visibility. This 
> option is therefore required to convert the summarized (default public) vcall 
> visibility into hidden to get the prior more aggressive behavior they are 
> trying to test.
> 
> Note I could have instead changed the assembly here to add hidden 
> vcall_visibility metadata everywhere. That seemed a little onerous so that's 
> why I just added the option. I could add a comment to that effect if it would 
> help?
I think you can remove this option from this test (and probably others using 
-wholeprogramdevirt-summary-action=import option), because visibility seems to 
be not analyzed on import phase. I just did this btw and test still passes 
flawlessly.



Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

tejohnson wrote:
> evgeny777 wrote:
> > Hm, looks like I don't fully understand this. I have following concerns:
> > 
> > 1) According to your changes every time I use `opt -wholeprogramdevirt` I 
> > also have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` 
> > flag become no-op without this additional flag? If so this looks counter 
> > intuitive to me.
> > 
> > 2) When I use `opt -wholeprogramdevirt` default constructor of 
> > `WholeProgramDevirt` class is called and `UseCommandLine` flag is set to 
> > true. Can't I use this flag to effectively lower visibility in module 
> > instead of playing with metadata?
> > 
> > ```
> > if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
> > !UseCommandLine)
> >  return false;
> > ```
> > 
> > According to your changes every time I use opt -wholeprogramdevirt I also 
> > have to pass -whole-program-visibility. Has -wholeprogramdevirt flag become 
> > no-op without this additional flag? If so this looks counter intuitive to 
> > me.
> 
> No, it wouldn't be needed if the tests contained !vcall_visibility metadata 
> indicating hidden LTO visibility (see my earlier comment response).
> 
> > When I use opt -wholeprogramdevirt default constructor of 
> > WholeProgramDevirt class is called and UseCommandLine flag is set to true. 
> > Can't I use this flag to effectively lower visibility in module instead of 
> > playing with metadata?
> 
> I could do that. What it would mean though is that we would be unable to use 
> opt for any future testing of vtables intended to have public vcall 
> visibility (either through a lack of that metadata, or explicit 
> vcall_vsibility metadata indicating public). Which might be ok - in fact all 
> my new testing of this behavior is via llvm-lto2 or the linkers. I suppose 
> that would obviate this change as well as all the opt based test changes to 
> pass the flag. Do you think that is better?
> I could do that. What it would mean though is that we would be unable to use 
> opt for any future testing of vtables intended to have public vcall 
> visibility (either through a lack of that metadata, or explicit 
> vcall_vsibility metadata indicating public). Which might be ok - in fact all 
> my new testing of this behavior is via llvm-lto2 or the linkers. I suppose 
> that would obviate this change as well as all the opt based test changes to 
> pass the flag. Do you think that is better?

Thanks for explanation. I think it's okay to have this extra option for

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-16 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 238569.
tejohnson added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_alias.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_external_comdat_same_guid.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
  llvm/test/Transforms/WholeProgramDevirt/import-no-dominating-assume.ll
  llvm/test/Transforms/WholeProgramDevirt/import.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -625,6 +626,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp
===
---

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-16 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson marked 2 inline comments as done.
tejohnson added inline comments.

Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

evgeny777 wrote:
> Why do you need `-whole-program-visibility` here? Correct me if I'm wrong, 
> but AFAIK module scanning doesn't happen during import and GV visibility 
> should be taken from imported summary.
Before my patch, type tests were only inserted for vtables with hidden LTO 
visibility. Therefore, the very presence of type tests communicated the hidden 
visibility and enabled the WPD.

With this patch, to support enabling WPD aggressively at LTO time, we now 
insert type tests unconditionally under -fwhole-program-vtables. The 
vcall_visibility metadata tells LTO how to interpret them. And the new options 
allow changing those to hidden visibility to get the more aggressive WPD.

Because these legacy tests have type tests but no vcall_visibility metadata, we 
now will conservatively treat them as having public LTO visibility. This option 
is therefore required to convert the summarized (default public) vcall 
visibility into hidden to get the prior more aggressive behavior they are 
trying to test.

Note I could have instead changed the assembly here to add hidden 
vcall_visibility metadata everywhere. That seemed a little onerous so that's 
why I just added the option. I could add a comment to that effect if it would 
help?

Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

evgeny777 wrote:
> Hm, looks like I don't fully understand this. I have following concerns:
> 
> 1) According to your changes every time I use `opt -wholeprogramdevirt` I 
> also have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` flag 
> become no-op without this additional flag? If so this looks counter intuitive 
> to me.
> 
> 2) When I use `opt -wholeprogramdevirt` default constructor of 
> `WholeProgramDevirt` class is called and `UseCommandLine` flag is set to 
> true. Can't I use this flag to effectively lower visibility in module instead 
> of playing with metadata?
> 
> ```
> if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
> !UseCommandLine)
>  return false;
> ```
> 
> According to your changes every time I use opt -wholeprogramdevirt I also 
> have to pass -whole-program-visibility. Has -wholeprogramdevirt flag become 
> no-op without this additional flag? If so this looks counter intuitive to me.

No, it wouldn't be needed if the tests contained !vcall_visibility metadata 
indicating hidden LTO visibility (see my earlier comment response).

> When I use opt -wholeprogramdevirt default constructor of WholeProgramDevirt 
> class is called and UseCommandLine flag is set to true. Can't I use this flag 
> to effectively lower visibility in module instead of playing with metadata?

I could do that. What it would mean though is that we would be unable to use 
opt for any future testing of vtables intended to have public vcall visibility 
(either through a lack of that metadata, or explicit vcall_vsibility metadata 
indicating public). Which might be ok - in fact all my new testing of this 
behavior is via llvm-lto2 or the linkers. I suppose that would obviate this 
change as well as all the opt based test changes to pass the flag. Do you think 
that is better?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-16 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added a comment.

Thanks, I'm still in process of testing (now fixing issue which however is most 
likely related to devirtualization itself, not to this patch). Meanwhile some 
of my comments below.




Comment at: llvm/test/Transforms/WholeProgramDevirt/import-indir.ll:2
 ; Test that we correctly import an indir resolution for type identifier 
"typeid1".
-; RUN: opt -S -wholeprogramdevirt -wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility 
-wholeprogramdevirt-summary-action=import 
-wholeprogramdevirt-read-summary=%S/Inputs/import-indir.yaml 
-wholeprogramdevirt-write-summary=%t < %s | FileCheck %s
 ; RUN: FileCheck --check-prefix=SUMMARY %s < %t

Why do you need `-whole-program-visibility` here? Correct me if I'm wrong, but 
AFAIK module scanning doesn't happen during import and GV visibility should be 
taken from imported summary.



Comment at: llvm/tools/opt/opt.cpp:634
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ 
false);

Hm, looks like I don't fully understand this. I have following concerns:

1) According to your changes every time I use `opt -wholeprogramdevirt` I also 
have to pass `-whole-program-visibility`. Has `-wholeprogramdevirt` flag become 
no-op without this additional flag? If so this looks counter intuitive to me.

2) When I use `opt -wholeprogramdevirt` default constructor of 
`WholeProgramDevirt` class is called and `UseCommandLine` flag is set to true. 
Can't I use this flag to effectively lower visibility in module instead of 
playing with metadata?

```
if (VS->vCallVisibility() == GlobalObject::VCallVisibilityPublic && 
!UseCommandLine)
 return false;
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-14 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson marked 9 inline comments as done.
tejohnson added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:559
+  if (!CodeGenOpts.ThinLTOIndexFile.empty())
+MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr,
+ /*ImportSummary=*/nullptr,

evgeny777 wrote:
> Test case?
See  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp. Specifically 
the second block of clang invocations:

```
// Also check type test are lowered when the distributed ThinLTO backend clang
// invocation is passed an empty index file, in which case a non-ThinLTO
// compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert.

```
I'm testing both the new and old PMs, as well as the new PM O0 path (basically 
all paths modified in this file).



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

evgeny777 wrote:
> What caused this and other changes in this file?
Because we now will insert type tests for non-hidden vtables. This is enabled 
by the changes to LTO to interpret these based on the vcall_visibility metadata.



Comment at: llvm/include/llvm/Transforms/IPO.h:245
+ const ModuleSummaryIndex *ImportSummary,
+ bool StripAll = false);
 

evgeny777 wrote:
> s/StripAll/DropTypeTests/g ?
Woops, missed this one after my rename! Good catch.

Also, noticed the description in the comments is now stale, fixed.



Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:550
+  // pipeline run below.
+  updateVCallVisibilityInModule(*MergedModule);
+

evgeny777 wrote:
> I'd rather use
> ```
> updateVCallVisibilityInModule(*MergedModule,  /* 
> WholeProgramVisibilityEnabledInLTO */ false)
> ```
> and remove default value for second parameter
Good idea, changed.



Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1775
+if (TypeTestFunc) {
+  for (auto UI = TypeTestFunc->use_begin(), UE = TypeTestFunc->use_end();
+   UI != UE;) {

evgeny777 wrote:
> Fold identical code blocks
After looking at this code again, I have changed it a bit. We should only be 
removing assumes that are consuming type test instructions. I have changed this 
(which removes the redundancy in any case). I also added a check to the test 
mentioned earlier for this handling 
(clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp) to ensure we 
don't remove unrelated llvm.assume.



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:145
+/// when enabled via the linker.
+cl::opt DisableWholeProgramVisibility(
+"disable-whole-program-visibility", cl::init(false), cl::Hidden,

evgeny777 wrote:
> Is this tested?
Added a test of it to llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-14 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson updated this revision to Diff 238048.
tejohnson marked 2 inline comments as done.
tejohnson added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
  llvm/test/Transforms/WholeProgramDevirt/import-no-dominating-assume.ll
  llvm/test/Transforms/WholeProgramDevirt/import.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-uses-this.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-begin.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
  llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-end.ll
  llvm/test/Transforms/WholeProgramDevirt/vtable-decl.ll
  llvm/test/tools/gold/X86/devirt_vcall_vis_public.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -54,6 +54,7 @@
 #include "llvm/Transforms/Coroutines.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
+#include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Debugify.h"
 #include 
@@ -626,6 +627,13 @@
 return 1;
   }
 
+  // Enable testing of whole program devirtualization on this module by invoking
+  // the facility for updating public visibility to linkage unit visibility when
+  // specified by an internal option. This is normally done during LTO which is
+  // not performed via opt.
+  updateVCallVisibilityInModule(*M,
+/* WholeProgramVisibilityEnabledInLTO */ false);
+
   // Figure out what stream we are supposed to write to...
   std::unique_ptr Out;
   std::unique_ptr ThinLinkOut;
Index: llvm/tools/gold/gold-plugin.cpp
===
--- llvm/tools/gold/gold-plugin.cpp
+++

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2020-01-13 Thread Eugene Leviant via Phabricator via cfe-commits

evgeny777 added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:559
+  if (!CodeGenOpts.ThinLTOIndexFile.empty())
+MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr,
+ /*ImportSummary=*/nullptr,

Test case?



Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"

What caused this and other changes in this file?



Comment at: llvm/include/llvm/Transforms/IPO.h:245
+ const ModuleSummaryIndex *ImportSummary,
+ bool StripAll = false);
 

s/StripAll/DropTypeTests/g ?



Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:550
+  // pipeline run below.
+  updateVCallVisibilityInModule(*MergedModule);
+

I'd rather use
```
updateVCallVisibilityInModule(*MergedModule,  /* 
WholeProgramVisibilityEnabledInLTO */ false)
```
and remove default value for second parameter



Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+  // via the internal option. Must be done before WPD below.
+  updateVCallVisibilityInIndex(*Index);
+

ditto



Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1775
+if (TypeTestFunc) {
+  for (auto UI = TypeTestFunc->use_begin(), UE = TypeTestFunc->use_end();
+   UI != UE;) {

Fold identical code blocks



Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:145
+/// when enabled via the linker.
+cl::opt DisableWholeProgramVisibility(
+"disable-whole-program-visibility", cl::init(false), cl::Hidden,

Is this tested?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913



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

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

2019-12-26 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson created this revision.
tejohnson added reviewers: pcc, evgeny777, steven_wu.
Herald added subscribers: dang, dexonsmith, MaskRay, hiraditya, arichardson, 
inglorion, Prazek, emaste.
Herald added a reviewer: espindola.
Herald added projects: clang, LLVM.

Third part in series to support Safe Whole Program Devirtualization
Enablement, see RFC here:
http://lists.llvm.org/pipermail/llvm-dev/2019-December/137543.html

This patch adds type test metadata under -fwhole-program-vtables,
even for classes without hidden visibility. It then changes WPD to skip
devirtualization for a virtual function call when any of the compatible
vtables has public vcall visibility.

Additionally, internal LLVM options as well as lld and gold-plugin
options are added which enable upgrading all public vcall visibility
to linkage unit (hidden) visibility during LTO. This enables the more
aggressive WPD to kick in based on LTO time knowledge of the visibility
guarantees.

Support was added to all flavors of LTO WPD (regular, hybrid and
index-only), and to both the new and old LTO APIs.

Unfortunately it was not simple to split the first and second parts of
this part of the change (the unconditional emission of type tests and
the upgrading of the vcall visiblity) as I needed a way to upgrade the
public visibility on legacy WPD llvm assembly tests that don't include
linkage unit vcall visibility specifiers, to avoid a lot of test churn.

I also added a mechanism to LowerTypeTests that allows dropping type
test assume sequences we now aggressively insert when we invoke
distributed ThinLTO backends with null indexes, which is used in testing
mode, and which doesn't invoke the normal ThinLTO backend pipeline.

Depends on D71907  and D71911 
.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71913

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
  clang/test/CodeGenCXX/lto-visibility-inference.cpp
  clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
  clang/test/CodeGenCXX/type-metadata.cpp
  lld/ELF/Config.h
  lld/ELF/Driver.cpp
  lld/ELF/LTO.cpp
  lld/ELF/Options.td
  lld/test/ELF/lto/devirt_vcall_vis_public.ll
  llvm/include/llvm/LTO/Config.h
  llvm/include/llvm/Transforms/IPO.h
  llvm/include/llvm/Transforms/IPO/LowerTypeTests.h
  llvm/include/llvm/Transforms/IPO/WholeProgramDevirt.h
  llvm/lib/LTO/LTO.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/lib/LTO/ThinLTOCodeGenerator.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/test/ThinLTO/X86/cache-typeid-resolutions.ll
  llvm/test/ThinLTO/X86/cfi-devirt.ll
  llvm/test/ThinLTO/X86/devirt-after-icp.ll
  llvm/test/ThinLTO/X86/devirt.ll
  llvm/test/ThinLTO/X86/devirt2.ll
  llvm/test/ThinLTO/X86/devirt_available_externally.ll
  llvm/test/ThinLTO/X86/devirt_promote.ll
  llvm/test/ThinLTO/X86/devirt_promote_legacy.ll
  llvm/test/ThinLTO/X86/devirt_single_hybrid.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_hidden.ll
  llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll
  llvm/test/Transforms/WholeProgramDevirt/bad-read-from-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel-threshold.ll
  llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll
  llvm/test/Transforms/WholeProgramDevirt/constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl-check.ll
  llvm/test/Transforms/WholeProgramDevirt/devirt-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/expand-check.ll
  llvm/test/Transforms/WholeProgramDevirt/export-nothing.ll
  llvm/test/Transforms/WholeProgramDevirt/export-single-impl.ll
  llvm/test/Transforms/WholeProgramDevirt/export-uniform-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unique-ret-val.ll
  llvm/test/Transforms/WholeProgramDevirt/export-unsuccessful-checked.ll
  llvm/test/Transforms/WholeProgramDevirt/export-vcp.ll
  llvm/test/Transforms/WholeProgramDevirt/import-indir.ll
  llvm/test/Transforms/WholeProgramDevirt/import-no-dominating-assume.ll
  llvm/test/Transforms/WholeProgramDevirt/import.ll
  llvm/test/Transforms/WholeProgramDevirt/non-constant-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/pointer-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/soa-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/struct-vtable.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval-invoke.ll
  llvm/test/Transforms/WholeProgramDevirt/uniform-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/unique-retval.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-accesses-memory.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-decl.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-no-this.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-non-constant-arg.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-too-wide-ints.ll
  llvm/test/Transforms/WholeProgramDevirt/vcp-type-mismatch.ll

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

27 matches

Site Navigation

Mail list logo

Footer information