In D82363#2506919, @StephenTozer wrote:

W.r.t the replacement of DBG_VALUE with DBG_VALUE_LIST, the current plan is to make that all happen in a patch following this stack. The patch itself should mostly be deletion of old code and simplifying where possible, renaming DBG_VALUE_LIST to DBG_VALUE, adding an extra operand to the new DBG_VALUE to represent directness, adding several tests, and updating existing tests as appropriate.

It's possible that the InstantiationGroups result is not supposed to be empty; in which case, something deeper is wrong, and I can abandon this patch and file a bug. Let me know.

I'd expect the pace of preparing the release to be basically the same regardless of start date, so the milestones would make more sense as start date plus N weeks.
If you want wiggle room for the start date, that's fine, anything that expresses "late January" and "late July" will work. I thought it was going to be more definitive, what with specific week numbers and all.

I think "Week number" is too ambiguous to be a guide. If January starts on the last day of the week, does that still count as week#1? What day does the week start on, anyway--much of the world starts the week on Sunday, much of the world starts the week on Monday.
"Fourth Tuesday in January/July" is unambiguous and makes everything easier to plan. Using "Start + N weeks" for the rest of the target dates is fine.

Catching up on this, sorry...

In D91734#2482318, @dblaikie wrote:

But, yes, we could possibly do better with more strategic is_stmt, but that's a big/complex piece of work to tackle.

In D91734#2480930, @dblaikie wrote:

I haven't looked closely, but I take it this one test modification seems reasonable to you? I guess we're stepping into some subexpressions on a function call that we previously didn't? (they didn't have any instructions attributed to them until now, but it's not unreasonable that they have them attributed - for materializing constants to pass to a function call when the function call/constants are split over multiple lines, etc)

This version of the patch avoids the weirdness I was seeing with prolog instructions in certain cases.

Change how PHIs are handled; if the operand has a debug location, use it, otherwise don't set a debug location. Then, flushLocalValueMap() will look at the first local-value instruction; if it doesn't already have a debug location, we copy the location from the first instruction after the local-value instructions (which should have the debug location of the original IR instruction).

Thanks Duncan! I (or someone) will play around with that and see what we need to do. May take a little while, as we're about to freeze a release and then go on break for two weeks, but good to know there's a straightforward path.

One thing this patch does, is make decisions about default behavior static. Meaning, the option behavior cannot depend on other options; specifically, it can't be based on the triple, which allows target-specific customization. PS4 certainly has cases where our defaults are different from the usual ones, and I'd kind of think that was true for other targets as well.

I've run this through our copy of the GDB suite (8.3, not sure how old that is). There are 10 differences, with and without the patch.

This is still showing as approved; I'm going to try "Request Review" to see if that helps.

Rebase and combine cf1cc774d and dc35368c, plus revise handling constants materialized due to PHIs.
I plan to do some build-time and object size measurements, similar to the first time around.
I plan to run this against the gdb test suite, or at least the local copy we have here at Sony, and post my findings.

Reopening, will upload a new diff in a moment that modifies how PHIs are handled. I intend to run the gdb suite on this, will post findings when I have them.

In D91734#2432988, @dblaikie wrote:

Yeah, it boils down to something like this:

void f1(const char*, const char*) { }
int main() {
  char prog[1];

  f1(prog,
     prog);
}

Without the patch, you step to the 'f1' line, and then step into or over the function call (step or next).
But with these patches applied - you step to the 'f1(' line, then the 'prog);' line, then back to the 'f1(' line, then into/over the function call.

Again, could be acceptable - if those arguments had specific non-trivial code in them (like other function calls) you'd certainly get that step forward/backward behavior - but it's notewhorthy that this is change would make more cases like that & it'd be good to understand why/if that's a good thing overall.

And with these changes together, breaking on the function breaks on line 5 (presumably because this is creating the constant used by the second '&&' which is on line 5) instead of line 3. Not the worst thing, but I imagine given Sony's interest in less "jumpy" line tables, this might not be super desirable.

It's breaking on an xorl %eax,%eax which is produced by the PHI at the end of the conditional expression, which now has the source location of the operator at the top of the expression tree, which is the second && operator, yeah. Not the best. (FTR, the jumpiness complaints we get are usually more egregious, hopping between different source statements somewhat arbitrarily; not sure anyone would complain too loudly about hopping around within an expression. We see less of that in any case, because we suppress column info, but still.)

See D92606 for a front-end patch to improve locations in the IR.
That, plus reapplying this patch, should help out GDB. I haven't had a chance to run the suite myself with both patches applied and I'm basically off tomorrow, so if @dblaikie doesn't have a chance, I'll do that on Monday.

Sometihng like this seems plausible to me:

Just for grins, change the && to || and see what happens...
The xorl becomes a movb $1 (no surprise there). But, that instruction no longer has line-0, instead it becomes part of the prologue.
This tells me that the xorl had an explicit line 0, while the 'movb $1' has no location (a subtle but important difference).

@rnk is correct that the phi's source location is propagating to the xorl. I modified the .ll file to put a different line number on the phi, and the xorl picked it up. This is clearly on the front-end to get right.

The constant materialization (xorl) obviously does not belong in the prologue; I think the instruction ordering with the patch is preferable.
But, clearly we'd rather not have it attributed to line 0, but the line it actually belongs to. I'll start looking into this. Offhand the reduced example appears to be not unreasonably large :-)

The patch deletes llvm/CMakeLists.txt, which is surely not something you want to do.

The tests all pass, but I wasn't *quite* daring enough to commit this without a second opinion.

Abandoned in favor of D91734.

In D91761#2407164, @jpienaar wrote:

In D91761#2407009, @probinson wrote:

I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect

SAME could be achieved without needing SAME, COUNT too, and NEXT could be folded in too by extending to multi-line regex and verifying no newlines so too DAG. So I believe there is a goal with directives to make checks readable for users & simple to interpret rather than just possible. Escaping is possible but obscures what is being tested and easy to get wrong.

I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect; the LITERAL feature doesn't meet that bar. It's a way to simplify something that you could do already. Not to say I'm going to insist on killing it, but I would like to hear more justification.

In D91043#2390223, @dblaikie wrote:

The fix seems fine to me - but I think there's something that at least I'm not understanding and would like to understand, since it's related to understanding the correctness of this fix: Why are there a bunch of other uses of %llc_dwarf in the DebugInfo/X86 tests - why don't they have the same problem as this test?

I don't reproduce the HWASan failure on my Linux.

Fix an lld test and two lldb tests.

In D91734#2403719, @rnk wrote:

Thanks! It looks like the presubmit/Harbormaster testing found some more test failures that need addressing.

FTR, I plan to remove the -fast-isel-sink-local-instructions option. I think there might be some now-unused fields as well, such as LastFlushPoint, that could be removed. These are NFC follow-ups that can be deferred, this patch is big enough as it is.

Some sort of pilot error on that first diff. This should be better.

Dammit. Visual Studio keeps running clang-format on me. I thought I had that turned off and fixed up the formatting but apparently not. I'll make sure those bits aren't in the next round.

See D91734 for an alternate approach as suggested by @rnk .

Thanks, Joel! It will likely take a few weeks for me to finish up the unittests part of my project, at which point this seems like a nice starting point for the FileCheck part. The basic infrastructure is definitely there.

Building Clang with Clang, adding the prototype patch improved build time by a barely perceptible 0.5%.

I prototyped a patch that would

• call flushLocalValueMap() at the top of selectInstruction()
• turn off the instruction-sinking stuff
• not erase the DbgLoc applied to local value instructions

I think we should also consider scrapping the local value map or maybe flushing it after selecting each instruction. I don't think we're getting a whole lot of value from sharing local values between multiple non-call instructions.

+ people who reviewed D43093.

Ummm the default expression-stack type is address-sized, and I'm not aware that we support machines with address sizes > 64 bits?
(DWARF 5 does have a typed stack, but IIUC this path is not used for DWARF 5.)

Adding @jdenny and @thopre who I think have been messing with lit recently.

I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!

I haven't touched the streamer side of AsmPrinter in a long time, but...
On PS4 we default to -fno-exceptions and while the section ends up named .eh_frame instead of .debug_frame the content is fine; we've had no complaints from our debugger folks. Aside from the introduction of the .cfi directives (which you do want), I'm not aware that there's any codegen consequence to this combination.
So, I'm not clear why a special streamer is really necessary.

Can we have a separate NFC refactor commit, to move that lump of code into its own emitCGProfile method? It might make the functional bits easier to follow. Also we have a private change in that area, and the ping-pong for this change has been a real headache.

TL;DR: It's all good.

In D68620#2276034, @Orlando wrote:

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

In D68620#2266531, @dblaikie wrote:

@probinson - do you have any state here? Whether this is an ongoing issue, whether the improvements to avoid unnecessary debug_loc have reduced the overhead sufficiently, etc?

Couple of nits and LGTM.

Whoa--that's the *help* text? Well, if that's the only documentation for options that there is, I guess it has to go there; but it seems pretty excessive for the (ideally concise) output of clang --help.

Looks okay (one grammar nit), probably worth waiting for someone else to chime in.

No test?

@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?

LGTM

It looks like you have been tweaking a number of related SizeOf() methods. Is it practical to have just one? In a parent class, or even as a free function (as the size computation depends only on form/format, and is not related to the class at all).

I agree with @grimar as it looks like the change is from a DWARFDataExtractor without relocations to one with relocations.
Regarding header order, in cases like this it's usual to do an NFC commit to sort the headers that are there, and then the functional patch just adds the new one in the right place.

Just wanted to say I agree this can be better than the Inputs/ idiom, especially for comparatively short inputs. We have some downstream tests that use a bunch of echo commands to generate a second file, but an extract utility would be cleaner IMO.

According to https://bugs.llvm.org/show_bug.cgi?id=46653#c3 there is no problem on x86-64, suggesting the fix is more likely to be in the AArch64 target instead of in generic code. It's not uncommon for passes to lose source locations, or fail to add them when they should, so that is more likely to be the cause of the AArch64 issue.

Wouldn't using .ifdef directives with --defsym on the command line work equally well? At least for .s files run through llvm-mc.
There are examples of this in the lit tests already, easy enough to find.

LGTM.

In D83462#2142036, @echristo wrote:

So the tuning here for SCE is also a "does not support" or something else?