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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
