In D94324#2487572, @aganea wrote:

What are the exact conditions to repro Nick's failure? On Fedora 32, I ran py llvm-lit sigpipe-handling.c before this patch and before rGbf401256edd00e921a5d3a0bf4cf6ee66ae51cd6 , the test suceeds, it should not, should it?

Reduce the test a bit further.

Looks good to me as-is. Thanks!

Fix a typo in the test.

Add a clang driver test.

The issue is that InitLLVM initializes its private PrettyStackPrinter before installing the one-shot default SIGPIPE handler: the PrettyStackPrinter itself eventually calls RegisterHandlers(), which runs before the SIGPIPE handler is installed and therefore doesn't register a SIGPIPE handler.

Thanks, I've landed this change as bf401256edd0.

In rG9a3f892d018238dce5181e458905311db8e682f5#856804, @aganea wrote:

Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

  if (Sig == SIGPIPE)
    if (auto OldOneShotPipeFunction =
            OneShotPipeSignalFunction.exchange(nullptr))
      return OldOneShotPipeFunction();

  raise(Sig); // Execute the default handler.
  return;
}

Unfortunately I can’t get to this right now, but will take a look first thing tomorrow morning.

In D93662#2479383, @jmorse wrote:

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

In D87302#2476048, @AndrewLitteken wrote:

In D87302#2277131, @vsk wrote:

CodeExtractor contains a utility to fix up debug info after extraction occurs (I believe it's called fixupDebugInfoPostExtraction). It's solving the same problem - is there any way to generalize/reuse that logic?

I believe it already uses it since the CodeExtractor is used to create the new functions in each case, but this is mainly for the function that moves them all together. The problem is that fixupDebugInfoPostExtraction transfers the information from the original function, which would not be correct in this case since we are merging the information from several different code locations into one. This also addresses @jroelofs disappointment, but since I'm not aware of any mechanism to say that one particular line in IR is from two different source code locations, I think it has to be dropped altogether for correctness.

In its current form, the rule for preserving debug locs doesn't seem to apply here (https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location). Imho we ought to revise the guidance there before proceeding with this patch (e.g. by adding some carveout for when the destination debug loc is the same). If you can share, it might also help to have a little context - unless there's some intervening optimization, the dwarf generator's location cascade should kick in in this situation?

Hi @xiangzhangllvm, apologies for the delay.

In D84467#2448934, @alanphipps wrote:

In D84467#2423636, @vsk wrote:

Thank you, lgtm!

Thank you! I also would like to ask if you could commit it for me as I don't have access for that. Note that a handful of tests have binary files which were not uploaded with the diff. What do you advise I do to get those up here so that I can commit it (unfortunately my system doesn't have arcanist setup and that would take some time). Thanks again for all of your help!

The error handling additions look good to me, but I don't have the background to lgtm InstrProfWriter::validateRecord. cc @xur.

Thank you, lgtm!

Hi @OikawaKirie, thanks for the patch! Would you mind splitting up this patch? This should make each individual patch easier to review, and make it possible for changes to land incrementally (& faster). Generally we ask for patches to be split up into self-contained units: it's not an exact science, but splitting things up by directory might be a reasonable heuristic to start with.

Simplify, using run_to_source_breakpoint.

Thanks, lgtm.

Make the dummy target per-debugger.

Delete some dead code.

Looks good to me.

In D90689#2371689, @ggeorgakoudis wrote:

In D90689#2371522, @vsk wrote:

In the example, what pass/component is responsible for creating %1 = bitcast i32* %0 to i8*?

If it's CodeExtractor itself, it would be nice to solve the problem earlier by getting rid of bitcasts which only exist to wire up lifetime markers. I tried doing that in 26ee8aff2, but had to revert (099bffe7f) because llvm doesn't support specializing its lifetime intrinsics on opaque pointer types. That's a bug, I think: if we fix that, perhaps this whole problem goes away.

OTOH, if this %1 = bitcast i32* %0 to i8* value comes from a frontend or another pass, I think the approach taken in this patch is reasonable. Please add a test (perhaps in the CodeExtractor unittest).

Many thanks for the quick reply @vsk! The bitcast is generated by the frontend, not by CodeExtractor. Our use case is outlining a merged parallel region that includes sequential code which include bitcasts like that. I am going to add a unit test.

I have two questions about the CodeExtractor.

1. I have second thoughts on changing the IR within the findAllocas. Should I create a helper data structure, e.g., ReplaceBitcastUses as in for SinkCands, and do the transformation inside the extractCodeRegion caller?
2. Is the assumption that findAllocas is always called before findInputsOutputs? If findAllocas is not called, my concern is that findInputsOutputs will report an output on the use of the bitcast although this output would be removed if findAllocas has been called. This problem does not appear for extractCodeRegion because it always calls findAllocas but there may be other modules using the CodeExtractor interface. I could change findInputsOutputs not to include bitcast uses to lifetime markers as outputs regardless of whether findAllocas has been called. Is that reasonable or does that create other problems?

In the example, what pass/component is responsible for creating %1 = bitcast i32* %0 to i8*?

Thanks, lgtm with two comments --

I might've missed it, but the debug location merging behavior could use a test. Here's one way to write it: https://godbolt.org/z/Yb9PY9.

Thanks, lgtm!

I'll revert.

It looks like verifyFunction asserts that the DISubprogram attached to any function, declaration or definition, is distinct:

Jonas walked me through some of the issues here offline:

• Looks like this makes testing depend on having a configured lldb-env tool, this will make it hard to run testing against an lldb root.
• If we need to launch ./bin/lldb in some other workflow, we can consider having lldb-env derive all the info it needs from a root lit.site.cfg.py.

Re: making the lit.site.cfg self-contained, IIUC the situation is that there are a couple places where we shell out via subprocess, but that you'd like to get rid of these? I wonder whether we could bundle up the necessary scripts along with lit.site.cfg instead.

My remark was not so much about the need for another tool but rather the need to configure it. Right now I can hand-write a lit.site.cfg.py, omit a bunch of values, and point lit to it. With this tool I'd need to also configure it (i.e. replacing @LLVM_USE_SANITIZER@ and @CMAKE_CXX_COMPILER@.

In D90276#2357978, @JDevlieghere wrote:

Can you explain the added value of the tool a bit more. I can see how it's nice to be able to run export $(lldb-env); path/to/lldb but I wonder how often you'd end up doing that. When I invoke lldb directly, it's mostly to debug a test case, in which case I'd need to run lldb under lldb and set the environment variables myself. It also means we have to configure an additional file which goes against my effort of making the lit.site.cfg file self contained (to be able to run the test suite against a different toolchain).

Test changes look reasonable to me!

Friendly ping.

@wallace thanks for winnowing the test objects. I left an inline suggestion about simplifying the error-handling in IntelPTInstruction. Other than that, mechanically this is looking good. Thanks!

Friendly ping. On the assumption that the debug inst cleanup in MergeBlockIntoPredecessor was intended to be a compile-time optimization, I propose making it opt-in as the performance impact can be unexpected. I.e., I suggest that we land D88928 soon-ish (tomorrow barring any concerns/objections).

In D89818#2344014, @bjope wrote:

Comparing with the D88928 patch (in its current state), that patch removes the RemoveRedundantDbgInstrs call from MergeBlockIntoPredecessor, but only replaces that with extra calls to RemoveRedundantDbgInstrs in a few places where MergeBlockIntoPredecessor is used (so afaict that patch currently removes a lot of DbgInstr cleanups?). IMO this patch makes it more of an active choice to decide if the removal can be deferred or not. With D88928 it is easier to forget about such cleanups.

To my knowledge, we're not relying on the fact that branch_weights are 32-bit for bitcode compatibility purposes, or for PGO. We do support using older .profdata files with newer builds, but (afaict) this change to branch_weights does not affect that.

In D89810#2343831, @Orlando wrote:

Hi @vsk, thanks for taking a look, I'll address your inline comments shortly.

On the topic of stale values after inlining+DSE: Could we teach the inliner to insert dbg.values in the same pattern as LowerDbgDeclare for parameters which are pointers to caller-local allocas?

• Actually fix the size of the DILocalVar.

• Use DW_OP_bit_piece to implement the legacy zext.