This is aspirational -- would "should" be more appropriate?

"should" sounds right to me - but open to other folks preferences/ideas about how this should be framed

Not sure I follow the fma example.

Let me rephrase the words in the document to see if we're both understanding them the same way: "IF multiple instructions are merged and the merged location is in a different block than one of the inputs - you /should/ (must, whatever) merge" - the implication is then if you have multiple instructions that don't change blocks, the location should /not/ be merged?

That doesn't sound like what I'd expect - in the fma example, if the * and - were from the same basic block, I'd still expect to merge the location rather than dropping or zeroing the location. So that scope information was preserved (eg: if * and - came from the same inline function, then the fma should still be attributed to that inline function).

Re: "the implication is then if you have multiple instructions that don't change blocks, the location should /not/ be merged" -- yep, we're on the same page about the proposed rule. Your point about expecting the right inline scope on merged instructions is compelling though. I'll fix this up.

Part of my confusion here was over how exactly SamplePGO works: can it correctly attribute a sample of a line 0 location to the scope it points to? Maybe that's moot, though, since relying on location cascade to assign locations to merged instructions could be more misleading than helpful.

Part of my confusion here was over how exactly SamplePGO works:

I don't actually know how it works.

can it correctly attribute a sample of a line 0 location to the scope it points to?

In theory I think such samples could be used, but fairly likely they aren't used.

(you could imagine a sample based profiler could look at all source lines within a sampled basic block - and mark all those source lines as sampled)

I believe that (at least how we use it in Sony) SPGO doesn't use a line-0 location, but snoops around looking for a real source location in the same block. Jeremy will poke a couple people who actually worked on it to try to get a more definitive answer tomorrow (they're in the UK).

When sampling an executable, the "hits" are addresses which must be mapped back to source locations using debug line info (i.e. addr2line). Any transform that moves or merges instructions without updating the debug location can therefore affect the quality of the sample. The standard example is tail-merging (which in fact was the first optimisation we fixed).

Imagine we have an if-then-else, where the "then" and "else" blocks end with the same instruction(s). Tail-merge will common the instructions in the successor block.

For block-reordering, we want to know if one side of the if-then-else is executed much more than the other. But if tail-merge has retained the original debug locations in the merged tail we will get an inaccurate sample. If the "then" location is used, hits on the common instructions will be attributed to the "then" block which is wrong. Likewise if the "else" locations were used. The location of the tail is ambiguous.

To fix this we introduced the "getMergedLocation" call. The initial version was very simple. If the locations were the same, either can be used. But if the locations are different, we can't give a location and an empty ("unknown") location is used.

At some point, this "unknown" location ends up generating a DWARF line-0 record. Now this is the bit I didn't directly work on so I don't know where it occurs. My memory is that the work was done by Paul, and that an "unknown" location at the start of a basic block forced a line-0. This did exactly what we wanted, and we rewrote our initial patch to tail-merge that explicitly created a line-0 location.

P.S. We did a presentation of this work at EuroLLVM 2017 (https://www.youtube.com/watch?v=ceCEXnuWdmo) which you can watch for more examples.

Rob, I think the most relevant question is: If there's a sample hit on an address that maps to line 0, what does SPGO do with it? Throw it away? Rummage around for a non-zero location?

This is handled by AutoFDO (which reads a "raw" sample and converts it into a sample with lines relative to the start of the function). It's 4 years since I hacked that code so I wanted to check to make sure.

Anyway, the answer is that samples for addresses that map to line 0 are simply thrown away...

How about: "Converting an if-then-else CFG diamond into a select. Preserving the debug locations of speculated instructions can make it seem like a condition is true when it's not (or vice versa), which can lead to a confusing single-stepping experience. The rule for :ref:dropping locations<WhenToDropLocation> should apply here." ?

Thanks for pressing on this, it's not terribly clear as-written. How about: "Block-local peepholes which delete redundant instructions, like `(sext (zext i8 %x to i16) to i32) => (zext i8 %x to i32). The inner zext` is modified but remains in its block, so the rule for preserving locations should apply."

sounds good!

That's a good point, e.g. I don't think the ASan pass bothers with setting line 0 locations. I'll try to split this up.