[PATCH] D148950: [LTO] Add test coverage for previous dllimport fix

Wed May 3 08:37:29 PDT 2023

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm

In D148950#4293068 <https://reviews.llvm.org/D148950#4293068>, @ormris wrote:

> No, I don't think we need to modify the original test. It's now used to test the changes in d2ef8c1f <https://reviews.llvm.org/rGd2ef8c1f2ca33457247be26374852573098553c7>.

Ah I see. Looks like that test was then actually testing the similar support in llvm/lib/Transforms/Utils/FunctionImportUtils.cpp (which is why it was affected by d2ef8c1f <https://reviews.llvm.org/rGd2ef8c1f2ca33457247be26374852573098553c7> aka D74751 <https://reviews.llvm.org/D74751>), and not the support in LTO.cpp added in D55627 <https://reviews.llvm.org/D55627>. Which makes me wonder if we now have any tests covering the removal of the dllimport in FunctionImportUtils.cpp... After looking, that was added in f5220fb68f2baef2e6d2e0bf11c12caa9e22ef29 and there was a test added with it (llvm/test/ThinLTO/X86/dsolocal_dllimport.ll). Can you make sure that test is still testing the handling in FunctionImportUtils.cpp, and also add a comment to llvm/test/LTO/Resolution/X86/local-def-dllimport.ll as to what it is actually testing (which I guess is that we *don't* remove the dllimport if we didn't mark a def as dso_local during the function import handling?).

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148950/new/

https://reviews.llvm.org/D148950