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

[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