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

[Teresa Johnson via Phabricator via llvm-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<bool> 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