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
