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
