In D57115#1368363, @mgrang wrote:

In D57115#1368350, @dblaikie wrote:

In D57115#1368345, @mgrang wrote:

In D57115#1368335, @dblaikie wrote:

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

I ran ninja check-all and did not see any failures. I haven't changed the places where algorithm is needed (like for std::fill, etc) and which introduced compile errors. I have also not touched examples, unittests and util/unittest directories.

How did you identify these "I haven't changed the places where algorithm is needed (like for std::fill, etc)" places? By visual inspection of every file that's been touched?

No, I removed the header from all files and then put back only where the compilation failed. I agree with your comment "removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header".

In D57115#1368345, @mgrang wrote:

In D57115#1368335, @dblaikie wrote:

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

I ran ninja check-all and did not see any failures. I haven't changed the places where algorithm is needed (like for std::fill, etc) and which introduced compile errors. I have also not touched examples, unittests and util/unittest directories.

"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)

I'm OK with this change from a "slightly reduces debug info size" but I hesitate to suggest that this isn't a thing your consumer should support & this "When an assembly file with multiple .loc directives with no interveaning instructions are assembled, GAS produces a DWARF line table that maps one instruction to multiple entries." sounds like a bit of a mischaracterization of how I'd read the line table entries in question - I think this line table doesn't "map one instruction to multiple entries" but instead has an empty entry that contains no instructions. Everything between one .loc and the next is at that first location - if there's nothing between them, then nothing is at that location. I suspect that's how other consumers are viewing this situation.

Seems good - thanks!

Thanks for filing the bug/reducing a test case - this change should include an automated test case that captures this issue (check for other tests that pass -gembed-source to see how this might be tested, possibly expanding the existing test case case or introducing a new one (test/CodeGen/debug-info-embed-source.c looks like the place to start)

Probably best to have someone else from AMDGPU land review this for your uses there - I'm just giving some high level at-a-glance stylistic coverage here.

No doubt @rsmith will have a more nuanced/accurate opinion on this than I do, but I would've thought the point of implicit modules is that they don't get moved around & it sounds like that's what this patch is suggesting/supporting, but maybe I haven't understood it fully?

In D56245#1345316, @rnk wrote:

I don't think this should be llvm_unreachable, I think it should be report_fatal_error. LLVM has some some support for out of tree backends, and they could plausibly call this when using a release build of LLVM as a library. If all the callers were in-tree, then calling this would represent an LLVM-internal logic error (forgetting to check hasRawTextSupport()), and unreachable or assert would be appropriate.

Not sure I'm following this all in detail & at least for me (someone not super familiar with the details here) it might've been a bit easier to review in smaller pieces (breaking up the refactoring to generalize the components of the discriminator into smaller bits, etc) but all good. Thanks!

Might be worth having a summary of the goals here (rather than only by reference to the design discussion thread) for review and later as commit history, etc.

Rename dumpArray to dumpArrayType
Implement & test behavior for subrange without any attributes (no low/high/count).

Pull out array printing into a helper function

In D55281#1334783, @probinson wrote:

In D55281#1334706, @dblaikie wrote:

I'm just still not sure the extra cl::opt is worth it, though - are you? Is the extra coverage significant/important?

I'd rather see a test break locally (because it's target-agnostic and uses the flag) than wait for some NVPTX-target bot to break after I commit.

In D55281#1334452, @probinson wrote:

In D55281#1333802, @dblaikie wrote:

Still a bit curious about the other discussion about flags, etc.

Well, you have better access to Eric than the rest of us....

In D55721#1333018, @probinson wrote:

In D55721#1331847, @dblaikie wrote:

In D55721#1331812, @probinson wrote:

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

Using [C] (C = count) for both the count-only and upper-bound-only cases suggests that we're being descriptive rather than carefully reflecting the attributes that are in the subrange DIE, which is fine. So, if an explicit LB matches the language default, then the display should also be [C] (where C = count if we have a count, or C = U + (1 - L) if we have upper_bound).

The only change you're suggesting is to detect when an explicit LB matches the language default, and treating that case the same as if the LB were not specified (in terms of how it's printed out)?

Sorry, no, in the course of editing the comment I lost the important part: When converting UB-only to Count, you need to correctly factor in the default LB: C = UB + (1 - DefaultLBForLanguage) otherwise the display for 1-based languages will be incorrect.

Then, since you have to know the default LB anyway, the rest seems not too hard.

Use language default lower bound to inform size calculation from upper_bound with no lower bound
Use [size] rendering when explicit lower bound matches language default

Recommitted this, predicating everything on !useSectionsAsReferences, which means the existing sections_as_references passes unmodified & I added another separate test to validate the change. Thanks again @ABataev for that pointer/fix! (also validated it against the internal failure that caught this too)

In D55281#1333620, @probinson wrote:

In D55281#1333552, @dblaikie wrote:

@aprantl @probinson - you folks haev any feelings about this? Looks like these flags are essentially unused (except for testing, which could equally be accomplished by tests that specify an NVPTX triple, I think (except on builds that aren't building an NVPTX backend)) & are a collection of features that are all only needed on NVPTX. I'd lean towards removing them all & gating this on NVPTX. Or on one flag rather than several fine-grained ones, if/when someone starts playing with a ptx assembler that can cope with these features.

But yeah, no big deal I guess - leads to a bit of confusion about who these flags are meant for, under what conditions some of these features might be enabled but not others, etc.

In a way this reminds me of the discussions around how to do "debugger tuning" and there were some very strong voices in favor of unpacking the tuning setting into separate flags. I believe that the DwarfDebug ctor is the only place that directly checks tuning, or that was the goal on the LLVM side anyhow.

In D55281#1333573, @ABataev wrote:

In D55281#1333552, @dblaikie wrote:

In D55281#1333515, @ABataev wrote:

In D55281#1333514, @dblaikie wrote:

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

But all the other things (ranges, inline strings, loc, etc) are predicated based on NVPTX directly, right? Seeing one part of it separated out as a flag makes me think that there are other users of that flag - but if they're all equally requirements/limitations of ptxas, seems to me they should be predicated/treated the same way (& if we had another user that ends up needing some of this, we could refactor that out/flag it at that point - and then the difference between the handling would be explained by the difference in the needs of the current NVPTX situation and whatever the new thing is?)

No, most of them are also controlled by the options like -dwarf-inlined-strings for inlined strings. They are just forced to et to true by default for NVPTX.

Fair enough - though I'm not really sure of the value of all the flags.

@aprantl @probinson - you folks haev any feelings about this? Looks like these flags are essentially unused (except for testing, which could equally be accomplished by tests that specify an NVPTX triple, I think (except on builds that aren't building an NVPTX backend)) & are a collection of features that are all only needed on NVPTX. I'd lean towards removing them all & gating this on NVPTX. Or on one flag rather than several fine-grained ones, if/when someone starts playing with a ptx assembler that can cope with these features.

But yeah, no big deal I guess - leads to a bit of confusion about who these flags are meant for, under what conditions some of these features might be enabled but not others, etc.

David, all the patches that introduced those options were reviewed and accepted by Eric Christopher.

In D55281#1333515, @ABataev wrote:

In D55281#1333514, @dblaikie wrote:

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

But all the other things (ranges, inline strings, loc, etc) are predicated based on NVPTX directly, right? Seeing one part of it separated out as a flag makes me think that there are other users of that flag - but if they're all equally requirements/limitations of ptxas, seems to me they should be predicated/treated the same way (& if we had another user that ends up needing some of this, we could refactor that out/flag it at that point - and then the difference between the handling would be explained by the difference in the needs of the current NVPTX situation and whatever the new thing is?)

No, most of them are also controlled by the options like -dwarf-inlined-strings for inlined strings. They are just forced to et to true by default for NVPTX.

But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?

Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.

In D55281#1333473, @ABataev wrote:

In D55281#1333472, @dblaikie wrote:

In D55281#1333470, @ABataev wrote:

In D55281#1333464, @dblaikie wrote:

In D55281#1333451, @ABataev wrote:

In D55281#1333445, @dblaikie wrote:

Hey Alexey - thanks for taking a look!

In D55281#1333365, @ABataev wrote:

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Fair enough & I'll work on that (want to reproduce the internal failure first to then test this approach works there too)

But I'd still like to understand the difference between these choices - given Google encountered an internal failure where testing NVPTX was insufficient but it looked like it was still related to LLVM's assembly being passed to ptxas - wouldn't these other conditions be incorrect in that case? (ie: assuming there's some non-NVPTX target that uses ptxas, and anything using ptxas needs to disable all these features (ranges, inline strings, etc, etc) - I would've thought these cases testing NVPTX directly would be bugs? Or are there different/varied reasons these things are disabled in some situations but not others?)

I need to make those tests more strict. Currently, they are using CHECK in all places, though we need CHECK-NEXT to catch the situation you ran into. It seems to me, you emitted the label somewhere in the debug info sections and the checks for NVPTX debug info missed this label.

Sure - sorry, I'm still trying to understand this better.

Targeting NVPTX itself enables the "UseSectionsAsReferences" as well as disabling UseRangeSection, UseInlineStrings, UseLocSection, etc...

I made made the change only on !NVPTX, but that seemed to be insufficient - some internal test failed anyway (so it was !NVPTX and still using ptxas, I guess?)

What I'm trying to understand is, if such a thing (ptxas but not NVPTX) - how does it function? Since it would not do the other NVPTX things like disabling UseRangeSection, UseInlineStrings, and UesLocSection.

Does that make sense as a question? Is there an answer?

Not all of your changes are guarded by !NVPTX. E.g. Asm->OutStreamer->EmitLabel(BeginLabel); is not, but it emits the label inside of the debug info section. ptxas does not support labels at all! It uses oversimplified DWARF2, where the references can be emitted only as section_name +- offset.
All new EmitLabel()s must be guarded by !UseSectionsAsReferences condition.
Also, you can manually run the tests for the NVPTX debug info and you will definitely find all that new emitted labels in the output. You need to ensure, that your changes do not add new labels in the output when UseSectionsAsReferences is set to true.

In D55281#1333470, @ABataev wrote:

In D55281#1333464, @dblaikie wrote:

In D55281#1333451, @ABataev wrote:

In D55281#1333445, @dblaikie wrote:

Hey Alexey - thanks for taking a look!

In D55281#1333365, @ABataev wrote:

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Fair enough & I'll work on that (want to reproduce the internal failure first to then test this approach works there too)

But I'd still like to understand the difference between these choices - given Google encountered an internal failure where testing NVPTX was insufficient but it looked like it was still related to LLVM's assembly being passed to ptxas - wouldn't these other conditions be incorrect in that case? (ie: assuming there's some non-NVPTX target that uses ptxas, and anything using ptxas needs to disable all these features (ranges, inline strings, etc, etc) - I would've thought these cases testing NVPTX directly would be bugs? Or are there different/varied reasons these things are disabled in some situations but not others?)

I need to make those tests more strict. Currently, they are using CHECK in all places, though we need CHECK-NEXT to catch the situation you ran into. It seems to me, you emitted the label somewhere in the debug info sections and the checks for NVPTX debug info missed this label.

In D55281#1333451, @ABataev wrote:

In D55281#1333445, @dblaikie wrote:

Hey Alexey - thanks for taking a look!

In D55281#1333365, @ABataev wrote:

NVPTX does not support labels in the debug info sections. All these new labels must be created/emitted only if DwarfDebug::useSectionsAsReferences() is false.

Is that the right name for the flag? If you reckon so - though I'm not sure it means a lot to me, personally. (

Also, a few places in lib/CodeGen/AsmPrinter/* use isNVPTX directly, not UseSectionsAsReferences - are they correct? (eg: UseRangeSection, UseInlineStrings, UseLocSection, some test for physical registers in updateSubprogramScopeDIE) What's the difference between these two things?

useSectionsAsReferences() does not allow to define inner symbols in the debug sections and force using the section names itself as references in other debug info sections (if it is supported). You need to use this one. Other flags are also required for the NVPTX debug info, but they control different format features.

Hey Alexey - thanks for taking a look!

(from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexing for multiple changes in rapid succession (such as hitting the "save all" button in some text editor - or having made a few other quick changes one after another maybe), but still provides reasonable up-to-date data for the user.

Sounds alright to me - thanks!

In D55721#1331812, @probinson wrote:

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

Using [C] (C = count) for both the count-only and upper-bound-only cases suggests that we're being descriptive rather than carefully reflecting the attributes that are in the subrange DIE, which is fine. So, if an explicit LB matches the language default, then the display should also be [C] (where C = count if we have a count, or C = U + (1 - L) if we have upper_bound).

In D36993#1328613, @probinson wrote:

In D36993#1328536, @dblaikie wrote:

FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.

I can imagine some scheme involving a tool that generates assembler-source constants by pulling in Dwarf.def, but it's harder to imagine getting that hooked into the test infrastructure somehow.

FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.

In D55234#1325837, @MaskRay wrote:

In D55234#1325804, @dblaikie wrote:

In D55234#1318754, @ruiu wrote:

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

FWIW linking times on smaller (than the very large) programs like Clang are still significant to many folks (including LLVM developers themselves) - for example when running "ninja check-all" many LLVM tools (~30? I forget roughly how many) have to be linked - and saving time on each (or the longest one) means getting out of that very serial step in the check run (massively parallel compiles, serial links, then massively parallel test runs).

I second the point, but see my comment above. If we want to optimize further, I think we should try some one-entry cache and leverage the getVA() calling pattern, instead of using a hash table (which has been deleted by this patch). The hash table may actually do worse with some SHF_MERGE|SHF_STRINGS.

In D55234#1318754, @ruiu wrote:

I think you should focus on large programs. We don't really care about the marginal improvements or regressions for small programs because linking small program is very fast anyway, and we don't care about 10ms improvements or regressions. For large programs, it seems reasonably positive. Also this patch only deletes code.

Overall I'm not sure this construct/pattern improves readability, but not too fussed either way.

From what I'm seeing, I would imagine this may regress debug info quality a bit. In the narrow example given, the DWARF still ends up with a description of 'foo', but without any location/value - though this is the same as if 'foo' was in the same translation unit (looks like LLVM fails to track the removing of the global and replacing everything with a constant - I believe the debug info metadata supports describing such a situation, but it isn't being used here - with or without this new change). SO that's not a regression.

In D55309#1320279, @evgeny777 wrote:

Could you tell me the commands I need to run to test this?

You can try:

clang -flto=thin -O3 main.c foo.c -fuse-ld=lld

In D55309#1319764, @evgeny777 wrote:

Hello David,

This is an example of importing read-only GV:

// main.c
extern int foo;
int main() { return foo; }

// foo.c
int foo = 42;

The variable foo is imported to main TU and internalized.

Great, thanks!

Ah, I found a comment from the original review:

I'm unaware of any regression for debug info quality that occurs with this change, but I haven't done wide scale testing of it - does anyone have a deeper/practical/feature test that demonstrates the motivation for this part of the change in the first place?

In D55281#1318934, @JDevlieghere wrote:

What happens today with NVPTX for other debug info sections that use label arithmetic? I know for sure that the accelerator tables/debug_names is using this, but maybe they're not used with this target?

In D55281#1318923, @ABataev wrote:

This change is not compatible with NVPTX, it does not support any of the label arithmetics.