tejohnson marked 4 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: > tejohnson wrote: > > 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? > I think you can remove this option from this test (and probably others using > -wholeprogramdevirt-summary-action=import option), because visibility seems > to be not analyzed on import phase. I just did this btw and test still passes > flawlessly. Good point, removed from here and a couple other similar tests. I added it a little overeagerly to address the failures I got originally. ================ Comment at: llvm/tools/opt/opt.cpp:634 + // not performed via opt. + updateVCallVisibilityInModule(*M, + /* WholeProgramVisibilityEnabledInLTO */ false); ---------------- evgeny777 wrote: > tejohnson wrote: > > 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? > > 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? > > Thanks for explanation. I think it's okay to have this extra option for > devirtualization to work with legacy IR files using opt. But please add > comment somewhere documenting why exactly this option is needed (probably > near its definition in WholeProgramDevirt.cpp) Added a comment documenting the need to where the option is defined. 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
