I was looking at a benchmark for glyph rendering in WebKit. Unfortunately, the file appears to reference headers from an Apple-internal SDK, and I can't find an analogous benchmark in open source. I've stashed the bitcode for now: happy to benchmark future patches against it if requested.

Hi Vedant,

LGTM

LGTM otherwise

LGTM

(NB, I haven't looked at the code as it seems you're requesting feedback for now,)

LGTM

LGTM with the remaining inline comments addressed, cheers.

Would it not be possible to override handle_options in the tool class, put the logic from _verify_options there (raising an Error if there's a problem), and then call super(Tool,self).handle_options() if there's no error? This would avoid the need to add a new method / code path.

Thanks for pushing through and finding out how to improve co-routine variable locations Brian; I have a few comments inline. Alas, I can't help with the lldb tests.

Looking good; I now see that IntervalMap will fire an assertion if an overlapping insert is made, I'd previously thought it would just be inefficient, this explains why CoalescingBitVector can't do it.

Nice, I can think of a few other places where this could prove useful (the register coalescing code for variable locations for starters). Some comments inline, plus

• |= / set union could do with being covered in the unittests too,
• Maybe some allocations could be avoided by using things like IntervalMap iterator setStartUnchecked and similar, but this looks good regardless.

I've added some unit tests; without the patch to LexicalScopes.cpp, these failures occur:

/me rustles around some drawers,

LGTM landed whichever way; am I right in thinking that this approach will still be faster than the domtree query even if the block is very large? (On account of the pointer set lookup being ordered, instead of walking through each instruction in a block).

Sounds good, how's this?

Greg via email points out vswhere: https://devblogs.microsoft.com/setup/vswhere-is-now-installed-with-visual-studio-2017/ which seems to be just what the doctor ordered. I'll take a look at that shortly (and land this patch).

Hi, sorry for the long delay,

This generally sounds fine to me, I have some high level comments: if this functionality is likely to be repeated elsewhere, it might be better to use the DebugVariable class in DebugInfoMetadata.h. It'd be more concise to compare variable identities by equality of DebugVariable objects rather than by individual components. (Not necessary for this patch, but worth keeping in mind if there are similar upcoming patches).

It's not represented in the *data* at all since we only have a set, it just falls out of the algorithm. Can you remind me why this works again? Is it because we visit each block locally once before calling any join() function?

Adrian wrote:

My understanding is that for each DBG_VALUE d in the analyzed MIR function and each instruction i

• true means: "it is correct to insert d at position i"
• false means: "it would be wrong to insert d at position i"

In other words at each instruction i the set of DBG_VALUEs that are "true" are the set of "live" DBG_VALUEs.

Pushed up reverts in 6531a78ac4b5. I'll start looking at an actual fix for the -O0 path back in D68945.

Hmn -- it looks like a / the root cause of this is that the O0 pipeline has a different mechanism for handling DBG_VALUEs of spills [0], hence changing isIndirect for both compilation modes is causing trouble. The code path in [0] probably should have been updated in D68945, unfortunately I'm not familiar with O0, at all, and wasn't really aware of it. Fascinatingly it runs LiveDebugValues but not LiveDebugVariables...

In D73725#1852265, @aprantl wrote:

I'm not going to make you write one, but this would be a great opportunity ;-)

Ping (summary of situation in previous comment).

Adrian wrote:

We don't have an existing unit test for LexicalScopes that generates IR manually and tests a few common queries, do we?

(Heading through my email backlog),

Hi Chris,

Hi, I'm now getting around to committing this. The developer policy [0] now says I should edit your details into the "Author" field of the commit, I'm using your phabricator-registered email address and "Chris Ye" from your phab profile. If there are buildbot failures when the commit goes up (possibly unrelated to this change), you might get messages to that email address.

Address review comments; will drop an explanation inline.

LGTM, thanks for working through this. Do you still need patches to be committed for you?

I wrote inline:

Wouldn't a block that has no PHIs or labels end up returning MBB2.begin()? Applying std::prev to that would break.

Looking good, this is almost there (see inline),

I think I can narrow the conceptual difference between our interpretations down to this:

Look like adding one more method for SkipPHIsAndLabels with const_iterator would be more easier. I thought will be more duplicate code to implement SkipPHIsAndLabels and SkipPHIsLabelsAndDebug in MachineVerifier.

I try to add below reference code to MachineVerifier::checkPHIOps, but meet a build error. In the function the type of MBB.begin() is const_iterator, but type of SkipPHIsAndLabels argument is (iterator I). How to convert const_iterator to iterator and pass to SkipPHIsAndLabels? Could I write the MachineVerifier as below code?

If I understand correctly, the problem is that calling user_back() might access a container with zero elements, yes? This would appear to be an easy fix -- however IMO you should add reviewers who're familiar with the Reassociate pass better [0]. From a couple of minutes examination it would appear ShouldBreakUpSubtract is only callable from OptimizeInst, and that in turn is guarded by isInstructionTriviallyDead, which I would expect to skip such an instruction. There might be something more complicated broken that someone more familiar with reassociate might spot.

Thanks for the patch, comments inline. A couple of things in general:

LGTM (I'm not very familiar with entry values, so best to see what other reviewers thing too).

Great to hear that this is fixing the issue. I agree with Björn, there should be a MachineVerifier check too that DBG_VALUEs are not mixed with labels. That'll make it very easy to detect this issue reoccurring.

Too lazy on the earlier explanation there sorry. To elaborate: I thought I'd accounted for nondeterminism in the two loops in reinsertSunkDebugDefs, but both were a bit broken:

Remove some now needless variables, switch another DenseMap to MapVector. Having fluffed the explanation before, I'll reopen this momentarily with a fuller explanation,

I found a couple of dozens of cases building clang where this patch causes SelectionDAG::transferDbgValues() to move processed debug value past another debug value of the same variable and I'm going to investigate them in details but it can take some time.

For now, I think we could conservatively let such debug values be on their places not changing their order (although it doesn't seem necessarily correct) and add corresponding TODO or FIXME note for further investigation.

Here's a revised patch using SetVector, although it's not at the location I was expecting. As it turns out, the llvm::sort of DebugVariables I was using was actively harmful -- the operator< method of DebugVariable compares pointers, which is fine for DenseMaps, but not for ensuring a consistent order.

I tried out D71279 and D71283 individually and they both seem to fix the problem I saw. I haven't done very wide testing, but the mir-reproducer I had passes. Nice!

This risks changing the order in which variable assignments appear to occur, as there's no consideration of whether the new IROrder goes past a dbg.value intrinsic of the same variable.

D71283 Should (TM) fix it, but, the x86 test that I wrote and which seemed to replicate the issue, now no longer replicates the issue. Which is unfortunate.

(D71279 is a precursor, just getting the second patch up, but I continue to be stuck in the tar pit...)

Misery, not today, but I still think there should be an easy fix. Please do revert this patch though if it's causing difficulties.

Ouch, yeah, I can see exactly why that is: the sunk DBG_VALUE is copied from the old / "original" DBG_VALUE, which in the meantime has been copy-propagated. And through the copy propagation, it grows an extra piece of subregister information.