teresajohnson wrote: > Just updated a new revision. The major change are > > * The implemented feature will be on by default.
I think you mean "off" by default? Please do bring back the cl::opt so that we can disable it as necessary for debugging (we use '--lto-whole-program-visibility' extensively internally and would want a way to disable the new handling in case of issues in production builds). > Adding linker flag '--lto-whole-program-visibility' can enable this feature. > * Add more comments, fix a few minor issues, and rewrite for more cleanness. > * Remove all previous changes in ThinLTOCodeGenerator.cpp. llvm-lto2 can be > used to do testing. ThinLTOCodeGenerator.cpp is legacy LTO handling but still used by a few linkers (ld64 and afaik Sony's internal linker). It can be tested with llvm-lto. However, it does not currently support '--lto-whole-program-visibility' so I think it is ok to not support this there for now. I would just add a note to the PR description that this is only supported for the new LTO API for now. > * Add a few unit test cases. One request: please avoid force pushing changes when possible. It makes it impossible to use github's support for comparing to my last review or other old commits. See https://llvm.org/docs/GitHub.html#rebasing-pull-requests-and-force-pushes. Also, I will be traveling and OOO much of the next week, so my response time may be delayed again. I have just a couple of other suggestions on the current version though after a quick skim that I'll send shortly. https://github.com/llvm/llvm-project/pull/178587 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
