In D147620#4311265, @dblaikie wrote:

That's the fix at the source here - producers should produce declarations for these member functions and then it'll work.

I had changed my commit message to reflect that we're now skipping these, instead of handling them, but arc amend changed it back when I only meant to update the review status. I suppose I should have updated the title+summary here too -- sorry about that! For posterity, here's what I wanted it to say, dug out of my reflog:

Followed StephenTozer's suggestion to just skip tracking spills for DBG_VALUE_LISTs

I'm poking at unfamiliar code here, so I'm definitely not confident that I understand everything going on. I do know that this change is enough to fix the original Rust problems, which now pass without triggering any assertions. It's possible that the resulting debuginfo isn't actually correct though, if I understand your point about the state of LiveRegMap.

As long as the option is there for rustc to turn on, we can agree to disagree...

Ok, I see that you've gone ahead with that rewrite in D118685, so I'll abandon this change.

Make the recursion check the same for typed or opaque pointers

Note: I added RUNs for enough tests that I felt confident I had good coverage, but I didn't do all of them. I'm not really sure that the way I handled typed/opaque CHECK results is a good strategy.

Rust's LLVM fork has the revert, but this is still going to be a problem for Rust with system LLVM 13. Do we have a way forward for this release? A revert seems to be a safer path than trying to develop something new in short time.

If the SONAME is different in any way, it is totally distinct to the dynamic linker. There's no semver parsing or anything like that. That's all we can do from a technical perspective, and the rest is just communication in the release.

Rebase, and test that required VerifierPass is not bisected

Great, thanks for the update! I will rebase after that lands.

Works for me!

I tried the C reproducer from that Rust bug, and it turns out that alloca(0) was already fixed by D88548. But if I change that to a larger size, then it was skipping right over the probe loop. I confirmed that your change here does make it go through the loop.

I applied this all the way back to my original issue and confirmed that the coreos-installer test now passes -- thanks!
(I'll wait for the final version of this patch before I actually submit this to LLVM builds for Rust/Fedora/etc.)

Hmm, thanks for those pointers. To combine opt-bisect from the two pass managers, maybe there can be an explicit call to copy stateful instrumentation? I think it helps that they are not interleaved, so we could do MPM.run(), then somehow SI.copy_to(CodeGenPasses), then CodeGenPasses.run(). I still need to read that thread too...

Dropped Region from NPM opt-bisect

FWIW, I also tagged bug 47123 for release-11.0.0.

In D85691#2212015, @cuviper wrote:

Oh, when LLDB uses this flag, they have LLDBStandalone.cmake with include(CheckAtomic).

And you were observing this in an lld standalone build? Because otherwise we have llvm/CMakeLists.txt -> llvm/cmake/config-ix.cmake -> llvm/cmake/modules/CheckAtomic.cmake. So yeah, you should probably make sure this is included somehow.

include(CheckAtomic) for standalone builds

Oh, when LLDB uses this flag, they have LLDBStandalone.cmake with include(CheckAtomic).

Well, I now have evidence that this may indeed try to link a nonexistent library -- a new Rust CI run failed on MSVC:

@jfb You did see that this is only for NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB, right? So I'm only adding that link (and hidden locking) for targets that can't do 64-bit atomics natively, otherwise you get link errors like this Rust CI failure on arm. This kind of conditional atomic link exists in several places in the monorepo already.

More whitespace cleanup

In D85691#2208323, @jfb wrote:

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

Fixed indentation

Can you add tests that show what this is intended to look like for alignments greater than the probe step size?

LGTM, and I confirmed that it works in a full rustc bootstrap, thanks!

I applied this to the Rust fork to measure it in rust#72703, and here are the perf results. It looks like a reasonable gain -- no real effect on check/debug builds, but around 1% fewer instructions executed for optimized builds.

Ping for review, please.

I'll have to review that discussion of this pointers wrt Rust, since self can be different things, but it hasn't caused us problems yet AFAIK.

FWIW, here's more context for how I arrived at this:
https://github.com/rust-lang/rust/pull/57675#issuecomment-456210064

@glaubitz, I was specifically wondering if the author, @LionNatsu, would like to commit this on their own. I'm even more hesitant to intrude on your other review, where I wasn't involved at all.

In D51108#1232917, @LionNatsu wrote:

Thank you all very much. It seems all reviewers have now approved the revision. When will it be landed, just by the way? Did it miss the merge window (if there is one)? 😄

I don't think you need approval from all reviewers, as long as nobody actively disagrees. But FWIW, it looks good to me too.

• Removed the duplicate (and incorrect) declaration in debug-insts.ll debug_declare
• Added debug-cpp.ll which more directly targets this change.

In D51749#1226417, @rnk wrote:

I think this change could use a more direct test, consider starting with the IR generated for a C++ object, like:

struct NTCopy {
  NTCopy();
  NTCopy(const NTCopy &);
  int x;
};
int foo(NTCopy o) {
  return o.x;
}

Superseded by D50096.

D50018 was spurred by failures in the Rust testsuite, and I've confirmed this patch also resolves those failures. Thanks!

To be clear, this function actually combines SHL, SRA, SRL, and ROTL, so they should all be addressed accordingly. I only called out SHL because this was the particular one that bit me.