In D125994#3526014, @dexonsmith wrote:

Can you add a unit test for this? Ideally something that could expose errors if the tracking logic isn’t right, so probably a TempMDTuple or something assigned/moved between operands.

Can you add a unit test for this? Ideally something that could expose errors if the tracking logic isn’t right, so probably a TempMDTuple or something assigned/moved between operands.

In D122895#3511855, @aaron.ballman wrote:

(It also seems unfortunate to regress the false positive rate of this diagnostic before -fstrict-prototypes is available.)

-fno-knr-functions is available already today in trunk, so I'm not certain I understand your concern there (or were you unaware we changed the flag name perhaps?).

LGTM, thanks! One comment online below.

In D122895#3511649, @aaron.ballman wrote:

In D122895#3511611, @dexonsmith wrote:

Sure, I'm all for adding a new warning for users that want a pedantic warning. Can it be put behind a separate flag, such as -Wstrict-prototypes-pedantic, which isn't triggered by -Wstrict-prototypes?

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

Doing that would then make -Wstrict-prototypes effectively a no-op (it would still control -Wdeprecated-non-prototype I suppose?),

In D122895#3511611, @dexonsmith wrote:

Previously, -Wstrict-prototypes was useful for preventing actual bugs in code.

In D122895#3511376, @aaron.ballman wrote:

In D122895#3511312, @aaron.ballman wrote:

However, I think the blocks behavior is a bug based on this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L728 and I'd be more than happy to fix that, as I'm not checking that condition here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaType.cpp#L5579 which seems like a pretty obvious thing to be checking for before warning about not having a strict prototype.

I fixed this false positive bug in 4be105c98a9c7e083cd878ee1751e11160b97b4a, so blocks (and OpenCL) behavior should now be improved.

In D125488#3510320, @akyrtzi wrote:

In D125488#3510297, @dexonsmith wrote:

[To be clear, my question was because I don't see this patch deleting the code path that minimizes / saves-minimized sources. Can/should we delete the "minimize sources" code path?]

Oh, this is removed in the prior patch in the review stack (https://reviews.llvm.org/D125487)

In D125488#3510265, @akyrtzi wrote:

In D125488#3510214, @dexonsmith wrote:

Is there code in DepFS that can/should be deleted as part of this patch, or in a follow-up, or is it still around as an option?

Awesome to see this! Looking forward to the next step (using this in normal preprocessing!).

LGTM! Thanks for splitting this out (and fixing the MSan problem properly!).

In D124974#3504986, @porglezomp wrote:

Ah, so it'd be a test that passes pretty trivially on any bot that doesn't have a custom version of CLANG_DEFAULT_STD_C and CLANG_DEFAULT_STD_CXX like the default config, but could get caught by any other bots that do set it?

What's the best way to get the compiler to report the standard it's using for this test?

For C++ I can do:

clang++ -xc++ -dM -E - < /dev/null | grep cplusplus

And it will print e.g.

#define __cplusplus 199711L

But that requires quite some decoding to compare against CLANG_DEFAULT_STD_CXX which would look like LangStandard::lang_gnucxx98.

For additional context to my questions above, even though open source clang hasn't been using -Wstrict-prototypes, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects.

In D124974#3494374, @sammccall wrote:

In D124974#3494279, @dexonsmith wrote:

Can we add a test for this?

I think the problem with testing it is that this variable comes from the CMake config, and in the default configuration (as used by all the bots) it's not set.

This looks mostly correct to me, but perhaps pessimistic enough it'll regress performance unnecessarily. A few comments inline.

In D124878#3495317, @twd2 wrote:

In D124878#3492498, @dexonsmith wrote:

At a high-level, the design for writing bitcode is a two-pass traversal algorithm:

• First pass in ValueEnumerator indexes everything
• Second pass in BitcodeWriter writes things

It feels like this new logic could be put into the first pass in ValueEnumerator. (Probably the previous patch, which this builds on, should have done that as well, to avoid iterating over users() in BitcodeWriter, but I guess I didn't notice!)

I just tried your advice to update this patch (not uploaded yet) but found the existing logic is

for each function
    incorporateFunction, which enumerates function-local constants, including BlockAddresses, and we can maintain the users of referred foreign functions here
    writeFunction, which emits BLOCKADDR_USERS records and the function-local constants

However, for every function, before emitting BLOCKADDR_USERS records, we should expect that we have visited all the constants, including function-local ones, and thus we have collected all the users of the blockaddresses in the function.

Can we enumerate (and thus emit) all the constants at the beginning, or add a traversing loop dedicated to collecting the users of blockaddresses?
The latter will touch all the constants one more time.

Can we add a test for this?

In D124816#3494243, @dexonsmith wrote:

• AFAICT, every call to FileManager::setVirtualFileSystem() is fundamentally unsound unless the new FS is equivalent to the old one or the filemanager hasn't been used yet.

Two high-level comments:

In D112175#3489247, @dexonsmith wrote:

Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing push_back() since even on the toy benchmark it's not a big difference.

At a high-level, the design for writing bitcode is a two-pass traversal algorithm:

• First pass in ValueEnumerator indexes everything
• Second pass in BitcodeWriter writes things

In D112175#3489247, @dexonsmith wrote:

Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing push_back() since even on the toy benchmark it's not a big difference.

Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing push_back() since even on the toy benchmark it's not a big difference.

Adding @bnbarham, @jansvoboda11, and @benlangmuir, who have been looking at various FileManager issues recently. If you post a follow up I suggest keeping them in the loop!

If your host toolchain has C++17, I'd be curious if the regression goes away when you switch to that mode and use if constexpr.

In D112175#3481680, @jplayer-nv wrote:

[ RUN      ] StaticVsSmall/0.TimedStaticVector
[       OK ] StaticVsSmall/0.TimedStaticVector (371 ms)
[ RUN      ] StaticVsSmall/0.TimedSmallVector
[       OK ] StaticVsSmall/0.TimedSmallVector (596 ms)

Which demonstrates that simple insertions are significantly faster (almost 38%) with the StaticVector compared to a SmallVector which stays within its small buffer limit.

Let me amend this a bit... it looks like the reason SmallVector::push_back is slower in this case is because of insufficient specialization. Here's the trivially copyable version:

void push_back(ValueParamT Elt) {
  const T *EltPtr = reserveForParamAndGetAddress(Elt);
  memcpy(reinterpret_cast<void *>(this->end()), EltPtr, sizeof(T));
  this->set_size(this->size() + 1);
}

When Elt is passed by value, reserveForParamAndGetAddress() is unnecessary since the parameter will not alias with the SmallVector's buffer. This inhibits host compiler optimization. If we were to SFINAE split this function, we can match my StaticVector::push_back perf. The following SmallVector change works for me locally:

  template <typename RetT = std::enable_if_t<TakesParamByValue>>
  RetT push_back_impl(value_type Elt) {
    if (LLVM_UNLIKELY(this->size() >= this->capacity()))
      this->grow();
    memcpy(reinterpret_cast<void *>(this->end()), std::addressof(Elt),
           sizeof(T));
    this->set_size(this->size() + 1);
  }

  template <typename RetT = std::enable_if_t<!TakesParamByValue>>
  RetT push_back_impl(const_reference Elt) {
    const T *EltPtr = reserveForParamAndGetAddress(Elt);
    memcpy(reinterpret_cast<void *>(this->end()), EltPtr, sizeof(T));
    this->set_size(this->size() + 1);
  }

public:
  void push_back(ValueParamT Elt) { push_back_impl(Elt); }

Thanks for working on this! I think this will be a big improvement.

In D124635#3481346, @benlangmuir wrote:

LGTM, although I made a suggestion about the spelling.

In D123576#3467855, @dblaikie wrote:

I worry this is overly general (it's not obvious that any functor with boolean conversion has the semantics this patch relies on) & inconsistent with the standard library (std::function for instance doesn't provide this (or a narrower version of it) functionality - so we'll still have the bugs for any API that takes a function_ref and passes it to std::function)

In D122766#3450348, @dexonsmith wrote:

• Only canonicalize __FILE__ for the target platform when there's a command-line flag that suggests it's okay. I suggest adding -ffile-reproducible, which could be implied by -ffile-prefix-map but also specified separately when -ffile-prefix-map isn't being used.

In D122766#3450298, @ayzhao wrote:

So, the general consensus seems to be that we should use backslashes when targeting Windows.

I implemented using only backslashes for Windows; however, clang/test/SemaCXX/destructor.cpp fails when running on Linux with the following error (among other errors, but the one below is the most important).

...
error: 'error' diagnostics seen but not expected:
  Line 32: '<REDACTED_PATH_TO_LLVM_CHECKOUT_WITH_BACKSLASHES>\\clang\\test\\SemaCXX\\destructor.cpp' file not found
...

The reason for this is that the test has Clang target windows and the test also has the statement #include __FILE__.

One way to fix this would be to have every macro that accepts a path internally convert the path to the build environment's path format, but TBH I'm not sure. What do you all think?

In D67678#3446526, @rsmith wrote:

@dexonsmith @arphaman What do we need to do to get this re-landed?

LGTM, with a couple of nitpicks. If you'd rather not improve the tests that's probably fine, since the patch doesn't make them any worse.

In D123104#3443448, @dexonsmith wrote:

I wonder if https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 could be related. A long shot (I can't see how that could be it!), but the other not-upstreamed patches look even less relevant.

In D123104#3439261, @bnbarham wrote:

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

LGTM, with one nitpick inline!

LGTM!

In D123398#3439946, @bnbarham wrote:

Does clang/test/VFS/external-names-multi-overlay.c need to be formatted or is it fine? It uses split-file so I'd really like to avoid the format here (makes it pretty silly).

Nice! LGTM.

LGTM once @sammccall 's new comments are addressed!

In D123300#3437683, @aeubanks wrote:

$ bin/clang -fsanitize=memory -m64 -gline-tables-only ../../compiler-rt/test/msan/use-after-dtor.cpp -fno-sanitize-memory-use-after-dtor -o - -S -emit-llvm -Xclang -disable-llvm-passes
the debug location for main's return instruction is line 39 with typed pointers and line 50 with opaque pointers.

In D120781#3437465, @nickdesaulniers wrote:

there's some tests failing where the "use list" (???) seems to be incorrect

LGTM; thanks for iterating on it.

(Seeing the other replies now!)

Adding a couple of reviewers that have been looking at FileManager issues with me recently; maybe they have a different perspective.

The ugly part here:

• FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
• I changed this to use a FileManager as a factory

Once we've removed the last use of DirectoryEntry::getName(), we should drop DirectoryEntry::getName() to avoid picking up other users. Perhaps DirectoryEntry itself could be made private to the FileManager.

LGTM!

In D120714#3429834, @kovdan01 wrote:

@dexonsmith Please let me know if you see some issues in the patch. Thanks!

In D122766#3429552, @hans wrote:

My feeling is that the default behavior on Windows needs to be to use backslashes and not forward slashes.

Okay, how would folks feel about always canonicalizing __FILE__ etc. to use backslashes when targeting Windows?

In D121375#3429023, @sammccall wrote:

I only see one usage in-tree (and one more in clspv). And migration is very easy. I think you should do it all in this commit.

Nice!

(Forgot to click "accept".)

This LGTM, with a couple of comments (on the comments!) inline.

In D123103#3428298, @bnbarham wrote:

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

This makes sense to me! A few comments inline.

In D120714#3417193, @kovdan01 wrote:

@dexonsmith

In D120714#3400246, @dexonsmith wrote:

Thanks for the pings! I'd missed your updates.

In D120714#3377826, @kovdan01 wrote:

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

Which tests are you talking about? Can those tests be marked as UNSUPPORTED when ABI-breaking checks are off?

If not, why not?

It would be good to add a comment to the sources explaining why LLVM_ENABLE_ABI_BREAKING_CHECKS can't/shouldn't be used for PersistentId, and potentially a FIXME with how to fix the blockers such as they are. Probably next to the declaration of the field.

Currently there is no check for ABI-breaking changes (see llvm/test/lit.site.cfg.py.in). Of course, that could be easily added, but I decided to update the tests themselves to make them work with SDNode debug IDs both in t<number> and 0x<hex address> formats. Also changes update_llc_test_checks.py, and it looks like that we can support more test scenarios for isel in it (not only resulting selection DAG, but also initial and intermediate ones, instcombine phase, etc.)

Great; seems like maybe we'd be okay to max out at 16 operands in "small" mode.

LGTM.

In D122549#3412021, @bnbarham wrote:

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

This looks nice!

Thanks for thinking it over; SGTM. (I haven't reviewed details of this patch, but don't let my comment block landing it.)

I think dropping struct names from intrinsic return types is a bit unfortunate. I wonder if there's a way to keep them?

In D120781#3404235, @nickdesaulniers wrote:

So I think when BitcodeWriter is serializing a function, it can check whether each of the function's basicblocks has its address taken. If so, it can build the set of GlobalValues that are Users of that BlockAddress. I'm curious if this should be part of the record or an Abbreviation? (Not sure I fully understand what an abbreviation is, despite just reading https://llvm.org/docs/BitCodeFormat.html#abbreviations). It seems like adding a record is unconditional though? So not sure about making it optional/only emitted if necessary?

In D118416#3408550, @wolfgangp wrote:

I'm finally getting around to take a stab at this. One thing in particular is giving me trouble:

In D118416#3285239, @dexonsmith wrote:

• Ensure that an empty temporary or distinct node can be resized later by co-allocating enough space; as a side effect, even an empty node would start with some small storage, but that's probably okay. (An empty uniqued node wouldn't need anything co-allocated.)

I'm largely following your implementation strategy. On first allocation I'm creating all nodes they way they are currently, i.e. with co-allocated operands. For temporary and distinct nodes I'm allocating extra space for a potentially needed hung-off-storage descriptor, if the number of operands is small (i.e. the space allocated for them is not enough for what's needed for the descriptor). For uniqued nodes I don't allocate anything extra.

The resize operation is fairly straightforward to implement, but the trouble I'm having is at deallocation. Temporary MDnodes can be made unique, and later at deallocation time we can't distinguish between uniqued nodes that have originally been allocated as temporary nodes and the ones that were allocated as unique from the start. A possible solution would be to steal another bit from NumOperands (reducing it to 30 bits) and capture the original allocation state there, but I'm still looking for alternatives.

Cloning these small temporary nodes (without the extra size) at uniquing time would be another option, though it seems like the infrastructure is not available for this operation. I assume we don't want to saddle all MDnodes (even the uniqued ones) with the extra space.

At the moment I'm trying the additional bit approach, which seems to be working, but I wouldn't mind hearing your thoughts on this.

In D121733#3403995, @rnk wrote:

Ignoring the ".." path components for the moment, is this patch good to go as is? It doesn't affect that behavior.