In D94739#2502527, @dexonsmith wrote:

[...] maybe I can trigger [the] compile-time tracker myself [...]

LGTM! Thanks for splitting it out.

@fhahn, can you help with the review here? This is one of only a few instances of walking the use-list of ConstantData. The broader context is an old RFC that @willir is working with me on:
https://lists.llvm.org/pipermail/llvm-dev/2016-September/105157.html

Adding some reviewers that have touched the SLPVectorizer recently to check this. (I have context on the motivation for the change and planned changes to the IR, but I don't know this pass well.)

In D94844#2502725, @nathawes wrote:

Yeah, certainly. Let me put that up separately and update this diff.

This patch also refactors OverlayFileSystem's directory iterator implementation (OverlayFSDirIterImpl) and VFSFromYamlDirIterImpl into a single implementation, addressing a FIXME about their conceptual similarity.

In D94811#2502504, @JDevlieghere wrote:

Personally I'd just like to get rid of ExternalFSValidWD inside of shouldUseExternalFS and let the underlying FS take care of it, which I believe would better fit the abstraction but would be a change in behavior (as pointed out in https://reviews.llvm.org/D65677#inline-604694).

Remove some unnecessary templates in SmallVector.cpp. I just realized I hadn't uploaded this version of the patch, which is what I ran the perf on.

I did a spot check of code size / compile-time, capturing clang's binary size in KiB (78460KiB => 78484KiB) and 5 runs of compiling VirtualFileSystem.cpp.o (looks like it's in the noise):

78460	baseline/build/bin/clang
        3.84 real         3.74 user         0.09 sys
        3.85 real         3.75 user         0.09 sys
        3.83 real         3.73 user         0.09 sys
        3.84 real         3.73 user         0.10 sys
        3.84 real         3.74 user         0.09 sys
78484	invalidation/build/bin/clang
        3.83 real         3.73 user         0.09 sys
        3.87 real         3.77 user         0.09 sys
        3.83 real         3.73 user         0.09 sys
        3.83 real         3.72 user         0.10 sys
        3.90 real         3.80 user         0.09 sys

"baseline" is ceaf0110ff5e0c2de1f03d65d13703d34d0d5737, and "invalidation" has this patch applied on top.

In D94800#2502240, @nikic wrote:

I tested the original three patches plus this one, and the results look good (or at least in line with expectations): https://llvm-compile-time-tracker.com/compare.php?from=33be50daa9ce1074c3b423a4ab27c70c0722113a&to=972a64b1331d1b3022154297593837234a958d0b&stat=instructions

I missed a few words:

In D94811#2502192, @dexonsmith wrote:

What if someone does setWorkingDirectory() for /a/b/c/d.c followed by

"changing directory to"

/x, and then looks up the relative path d.c? Or changes directory to ../c?

I'm wondering about this:

Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
fallthrough.

A concern I have is that the round-tripping might be too low level since it requires each group of options to opt-in somehow. Having something at the top-level that doesn't have to be touched again would be easier to validate / understand, and might better serve the purpose of verifying that the lower-level details were implemented correctly by not depending on them to turn it on.

Note that this depends on https://reviews.llvm.org/D93781 and predecessors, which were reverted due to the lack of https://reviews.llvm.org/D94800.

@xbolva00, thanks for reporting the regression. After the performance analysis was finished in https://reviews.llvm.org/D91837, I split the patch up for more detailed review, and in responding to review comments I somehow lost the critical check against TakesParamByValue.

@xbolva00, adding you here as well, since you reported the regression in https://reviews.llvm.org/D93779.

In D94800#2501597, @dexonsmith wrote:

Actually, I'm going to commit this immediately and not wait for review. Having caught up on my email that I missed yesterday, there's an active compile-time regression that this should fix. See https://reviews.llvm.org/D93779 for details.

(Happy to respond to comments post-commit of course.)

Actually, I'm going to commit this immediately and not wait for review. Having caught up on my email that I missed yesterday, there's an active compile-time regression that this should fix. See https://reviews.llvm.org/D93779 for details.

In D93779#2499361, @nikic wrote:

Over in D91837 the reserveForAndGetAddress() implementation contained a check for TakesParamByValue that seems to be gone in this revision. My first guess is that this is the culprit, and we're now always performing the isReferenceToStorage() check.

Simplified emplace_back and addressed the concern I had with copying large POD-like types too often.

• Moved it back to SmallVectorImpl, only delegating to SmallVectorTemplateBase for growAndEmplaceBack().
• Now it only defers to push_back when growing (still just for POD-like types).
• Also improved the comment to explain why the copy makes sense even for large POD-like types: that it's better than losing the realloc optimization.

Note that this is a follow-up to https://reviews.llvm.org/D91837 (and the patches split out from it), and an alternative implementation for https://reviews.llvm.org/D87326.

In D93781#2470743, @dblaikie wrote:

Looks good (did you happen to try this with sanitizers without the fix applied & observe a sanitizer failure when running the updated test? Would be nice to know that's actually working as intended)

In D93779#2497213, @dexonsmith wrote:

In D93779#2497200, @rupprecht wrote:

This patch seems to have found a bug: in AArch64InstrInfo.cpp, there is a -Wsometimes-uninitialized warning that now triggers, and adding an assertion to make sure the value is eventually set causes a few tests to fail. The usage is DelInstrs.push_back(MUL), which I guess is how it ties in to this patch.

More details in 752fafda3dbf1f4885c64408a13ddb67c91ccb13, which gets things back to building, but doesn't do anything about the bug.

Oh weird, I saw that too, but (wrongly) assumed it was unrelated to my change. Thanks for digging into it.

In D93779#2497200, @rupprecht wrote:

This patch seems to have found a bug: in AArch64InstrInfo.cpp, there is a -Wsometimes-uninitialized warning that now triggers, and adding an assertion to make sure the value is eventually set causes a few tests to fail. The usage is DelInstrs.push_back(MUL), which I guess is how it ties in to this patch.

More details in 752fafda3dbf1f4885c64408a13ddb67c91ccb13, which gets things back to building, but doesn't do anything about the bug.

Thanks for the review! Landed in 3043e5a5c33c4c871f4a1dfd621a8839f9a1f0b3.

In D93779#2497112, @dblaikie wrote:

In D93779#2497099, @dexonsmith wrote:

Reverted in 56d1ffb927d03958a7a31442596df749264a7792 due to an error on Windows:
http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.

Guess maybe something in the std::is_same<std::remove_const_t<std::remove_reference_t<ArgType>>, T>::value isn't working quite right on MSVC? (could test that with godbolt, maybe) - is it needed? Since it's on both insert_one_maybe_copy and there's an implementation detail/doesn't need to protect from violations of that constraint (admittedly I'm proposing removing it because maybe MSVC is violating it... )? And instead SFINAE/enable_if only on the TakesParamByValue part?

Replacing insert_one_maybe_copy indirection with a forwarding helper called forward_value_param.

Reverted in 56d1ffb927d03958a7a31442596df749264a7792 due to an error on Windows:
http://lab.llvm.org:8011/#/builders/127/builds/4489/steps/7/logs/stdio
if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.

In D93953#2486016, @lanza wrote:

On a similar note, I noticed elsewhere after fixing this that you mentioned in a commit message that you intended to remove iplist and ilist and rename as owning_ilist (or something of that sorts). Is that still on the table?

It'd be much nicer to add the tests as part of the patch. I've added a comment to D94474 with a possible way to do that.

I'm wondering if we can get this in incrementally without needing to list in code which options are correctly handled. Here's one way to do it:

• Add a tool, clang-cc1-args, that shows the diff between round-tripping when given a set of args. (This could potentially grow other functionality vs. diffing. So maybe clang-cc1-args diff [args] would be the usage?)
• Add RUN lines to tests that test the args we think should round-trip correctly and confirm there's no diff. This can be done incrementally as options get handled.
• Only once we think we have all the options covered, add the assertion from this patch.

LGTM, with one nit. It'd be nice to add more detail to the commit message as well.

Can / should this be added to the catch-all test-depends as well? (I thought llvm-test-depends and clang-test-depends were both added there somewhere, although I admit I just the code and I don't see it...) Regardless, this could be done separately / later if it makes sense to do.

Thanks for the review! Committed with your suggestions in 5ccff5aaa68ab789834c4463ce05b05e57593b34.

Thanks for the review! (Just coming back to this now after the holidays.)

Rebase and respond to review feedback.

FWIW, that logic look corrects to me.

LGTM.

Please also update the commit message to explain why this is safe (that -fp-exception-behavior fully encapsulates the semantics for -cc1) rather than just saying the options were ignored (which sounds like a bug).

LGTM. I've just done a careful audit myself, and I'm now confident this patch is correct and that there is no latent bug -- that it's correct to ignore -f*trapping-math on the -cc1 command-line since -fp-exception-mode will always have a value matching / superseding -f*trapping-math.

(Marking as "Request Changes" to drop this from my queue; feel free to reach out if the direction I suggested isn't working well...)

With the above adjustment, this LGTM.

LGTM.

In D93981#2481315, @sanjoy.google wrote:

In D93981#2476868, @mehdi_amini wrote:

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

Wow, I'd have expected this to be decided by the C++ standard. This is from GCC I believe.

LGTM!

In D78058#2480411, @dexonsmith wrote:

In D78058#2471735, @sammccall wrote:

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

Thanks for looking.

In D78058#2471735, @sammccall wrote:

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

LGTM. Thanks! (Please reference the commit that removed allocation in the commit message.)

In D93701#2477239, @jansvoboda11 wrote:

This will make more sense after looking at D84673. In short: parsing of diagnostic options can happen outside of CompilerInvocation::createFromArgs, that's why ParseDiagnosticArgs is a free function. Ideally, we'd like to reuse the existing marshalling infrastructure here as well.

ParseDiagnosticArgs is usually called with DiagnosticEngine *Diags = nullptr. That's because the call happens early on during compiler execution, where we don't have a diagnostic engine yet. This call needs to happen first, to obtain the diagnostic options that are necessary for the engine instantiation.

This patch accommodates this free function by turning all DiagnosticEngine references to pointers. Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

In D93701#2479574, @jansvoboda11 wrote:

In D93701#2477239, @jansvoboda11 wrote:

Another solution would be to create a "dummy" DiagnosticsEngine for the ParseDiagnosticArgs calls, but I didn't find a way to do that.

Something like CompilerInstance::createDiagnostics(new DiagnosticOptions(), new IgnoreDiagnostics()) could work. I'll give that a try.

In D78058#2464262, @dexonsmith wrote:

I'll reply here once I've posted the RFC and patch (as I said, I'm hoping next week) so you can take a look.

Abandoning this in favour of:

• https://reviews.llvm.org/D93777
• https://reviews.llvm.org/D93779
• https://reviews.llvm.org/D93780
• https://reviews.llvm.org/D93781

Thanks again for the reviews here. For landing this (and a bit more review from @dblaikie) I've split this up into:

• https://reviews.llvm.org/D93777
• https://reviews.llvm.org/D93779
• https://reviews.llvm.org/D93780
• https://reviews.llvm.org/D93781

This is needed for a future patch, where we start using normalizers in contexts where no Diags are available.

LGTM.

LGTM, with one nit.

Thanks for the review!

Great, thanks for doing that analysis @nikic!