[PATCH] D127778: [LTO][ELF] Add selective --save-temps= option

[Jin Xin Ng via Phabricator via llvm-commits]

Northbadge added a comment.

In D127778#3609766 <https://reviews.llvm.org/D127778#3609766>, @tejohnson wrote:

> In D127778#3609617 <https://reviews.llvm.org/D127778#3609617>, @Northbadge wrote:
>
>> In D127778#3608725 <https://reviews.llvm.org/D127778#3608725>, @tejohnson wrote:
>>
>>> In D127778#3603536 <https://reviews.llvm.org/D127778#3603536>, @MaskRay wrote:
>>>
>>>> From a layering perspective, the feature is better tested on the llvm/ project, not lld/. You may need to change llvm-lto2 to check the values. Having tests in lld/test/ELF is fine too as that reflects how people use the functionality, but they should not be replacement for llvm/ side tests.
>>>> (llvm/test/ThinLTO/X86 and llvm/test/LTO/X86)
>>>
>>> This is a good point, and for D127779 <https://reviews.llvm.org/D127779> as well.
>>
>> I defer to the both of you on that point. However, this patch and D127779 <https://reviews.llvm.org/D127779> are more of an extension of `--save-temps` (in my opinion), and as such the tests are predicated on `--save-temps` being correct. I just assumed `--save-temps` was properly tested to begin with, but it seems to have only been tested within `ELF/lto` on a basic level. Would it be sufficient (for these 2 patches) to write llvm side tests for `--save-temps` as a separate patch?
>
> It's not the --save-temps option that should be tested by llvm-lto2, but rather the core LTO changes (which are linker independent)  being introduced in the patches. Typically when we add functionality to LTO we test it independently of any linker, via llvm-lto2, in one or both of the test directories @MaskRay mentioned above, then also add tests for whatever linker is utilizing the LTO support.
>
> I missed this in D127777 <https://reviews.llvm.org/D127777>, which should ideally also have some llvm-lto2 based testing. E.g. see the -thinlto-distributed-indexes option in llvm/tools/llvm-lto2/llvm-lto2.cpp which is testing the thinlto-index-only functionality (llvm-lto2.cpp also has a -save-temps option). There are a bunch of tests under llvm/test/ThinLTO/X86/ that utilize these options (and end up testing the related LTO functionality in the process).

Ah ok, thanks for clarifying that! I'll get on those right away along with a separate patch for D127777 <https://reviews.llvm.org/D127777> 's missing tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127778