debuginfo: Use symbol difference for CU length to simplify assembly reading/editing

[dblaikie]

Summary

Mucking about simplifying a test case ( https://reviews.llvm.org/D55261 ) I stumbled across something I've hit before - that LLVM's (GCC's does too, FWIW) assembly output includes a hardcode length for a DWARF unit in its header. Instead we could emit a label difference - making the assembly easier to read/edit (though potentially at a slight (I haven't tried to observe it) performance cost of delaying/sinking the length computation into the MC layer).

This does cause a few tests to fail currently - DebugInfo/X86/sections_as_references.ll (which was testing that there were no labels in the unit header & now there's one label there) and a few NVPTX ones (these might be a real issue - because NVPTX seems to have some strong limitations on what sort of assembly it can cope with - and maybe these label differences wouldn't be acceptable there, so I've added Alexey to this review to get his perspective on that part)

[JDevlieghere]

I'm in favor of using labels for the reasons you mentioned. It's also consistent with what we do elsewhere.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1559

Sometimes sizeof(uint32_t) is used here, but up to you.

[ABataev]

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

[dblaikie]

In D55281#1318923, @ABataev wrote:

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

Thanks for the heads up! If I go forward with it, I'll ensure this doesn't regress those NVPTX tests, then.

[JDevlieghere]

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?

[dblaikie]

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?

Yeah, it uses dwarf-2 or something & really restricts which features are enabled because of this limitation.

[probinson]

Easy enough to accommodate NVPTX, if you use DWARF version to pick constant size versus label diff. I don't remember what it takes to work out what the target is at this point, but version is obviously easy.

[ABataev]

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

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfFile.cpp

50 ↗

(On Diff #177599)

This also must not be emitted for NVPTX.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

1559 ↗

(On Diff #177599)

Labels are not supported by the NVPTX at all, you should not emit them unconditionally.

[dblaikie]

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?

[ABataev]

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.

[dblaikie]

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?)

[ABataev]

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.

[dblaikie]

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?

[ABataev]

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.

[dblaikie]

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.

Ah, OK - thanks for explaining. So my change was broken for NVPTX in general anyway, just a hole in testing as you mentioned.

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?

[ABataev]

In D55281#1333485, @dblaikie wrote:

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.

Ah, OK - thanks for explaining. So my change was broken for NVPTX in general anyway, just a hole in testing as you mentioned.

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.

[dblaikie]

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?)

[ABataev]

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.

[dblaikie]

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.

[ABataev]

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.

[dblaikie]

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.

Sure - doesn't mean it isn't worth some reflection/discussion - it's not a big deal either way. I don't mean to criticize you or Eric or anyone else involved in the work here, but to look at the end result & discuss whether it might be improved, on reflection.

[ABataev]

Paul Robinson also accepted some of the patches.

[probinson]

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.

I guess there are already feature flags for some of these ptxas quirks; the regular assembler doesn't care which way the flag is set, and ptxas depends on it being a certain way. I think as long as the regular assembler is agnostic about the setting, then having a feature flag instead of a triple check is the pattern we have been following. And that's the case here, right? Turning a hard-coded constant into an expression that the assembler will compute.
So I would lean in favor of a feature flag, defaulted based on the triple.

[dblaikie]

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.

I'm not sure that's the case - though I don't mind that as a concept/design. The debugger tuning is the input to DwarfDebug and then it gets handled one way or another (maybe split out into specific boolean flags describing narrower features to be enabled/disabled - HasAppleExtensionAttributes, UseAllLinkageNames, UseGNUTLSOpcoed, UseDwarf2Bitfields)

I guess there are already feature flags for some of these ptxas quirks; the regular assembler doesn't care which way the flag is set, and ptxas depends on it being a certain way. I think as long as the regular assembler is agnostic about the setting,

Kind of - at least for the useSectionsAsReferences flag, it looks like it'd only break if the user had data in any debug section (because this feature assumes that LLVM's the only one generating data into those sections, so it can compute how far from the start of the section it needs to go in this object file), which I guess is probably not super supported anyway?

then having a feature flag instead of a triple check is the pattern we have been following.

I'm not sure it is, though - lowering the triple (or debugger tuning - since that can't be accurately known based on the triple or other existing parameters) to boolean flags in DwarfDebug is common enough, but having a cl::opt to be able to set it through other means seems less common (we have some, but not all of the parameters configured via debugger tuning are exposed as their own cl::opt/-mllvm flag) & especially this cluster seems sufficiently narrow that I wouldn't expect other users to crop up wanting one or two of these things but not the rest. (& some of these NVPTX ones are configured only via the NVPTX target being specified, while a couple are accessible via -mllvm too)

UseInlineStrings - NVPTX or -mllvm -dwarf-inlined-strings
UseLocSection - !NVPTX only
DwarfVersion - NVPTX (version 2) or DwarfVersion module metadata
UseRangeSection - !NVPTX or -mllvm -no-dwarf-ranges-section
UseSectionsAsReferences: NVPTX or -mllvm -dwarf-sections-as-references

Limited use of DW_AT_frame_base depending on whether the frame register is a physical register, and not using DW_OP_call_frame_cfa - no cl::opt/-mllvm or bit in DwarfDebug, NVPTX is tested directly for this.

As for debugger tuning:
UseGNUTLSOpcode: GDB or DwarfVersion < 3
UseDwarf2Bitfields: GDB or DwarfVersion < 4
HasAppleExtensions: LLDB
UseAllLinkageNames: !SCE or -mllvm -dwarf-linkage-names
DwarfCompileUnit::hasDwarfPubSections(): GDB and some CU-level properties set the default

(& the tuning itself defaults based on target (darwin -> LLDB, PS4 -> SCE, otherwise GDB) if unspecified)

So, we don't have a super clear story by any means - and having the -mllvm/cl::opt flags means functionality can be tested without the target being built (which offers some broader test coverage, especially for developers who might remove targets to build to improve developer velocity, etc), but they also increase the physical and mental surface area of LLVM & I'd generally be inclined to remove them if they're only there for testing that can be achieved through target-specific tests instead. (eventually we'd probably want to get rid of most/all of the cl::opts anyway - library-fication and all that, but that's a long way off & we have other more functional flags (ones that aren't here, at best, for testing but mostly unused) that'll atke more work to plumb through debug info metadata, module flags, and MCOptions before we get there)

And that's the case here, right? Turning a hard-coded constant into an expression that the assembler will compute. So I would lean in favor of a feature flag, defaulted based on the triple.

[dblaikie]

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)

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

[probinson]

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....
Certainly having cl::opt flags makes it easier to test things in a non-platform-dependent way, if nothing else. I can see putting all the ptxas-special stuff under a single "use-ptxas-format" flag instead of several different ones. It makes the control less feature-oriented, but more obvious about the purpose/intent.

[dblaikie]

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....

Useful as a tie breaker, but I'm not sure it's that big of a deal we can't decide amongst ourselves. But yeah, since he approved the patches originally, might poke him about it at some point.

Certainly having cl::opt flags makes it easier to test things in a non-platform-dependent way, if nothing else. I can see putting all the ptxas-special stuff under a single "use-ptxas-format" flag instead of several different ones. It makes the control less feature-oriented, but more obvious about the purpose/intent.

I'm just still not sure the extra cl::opt is worth it, though - are you? Is the extra coverage significant/important?
I think the only reason we have a flag for the debugger tuning is that it can't always be deduced from the triple (eg: folks using LLDB on Linux, etc) - but this case can, it's always NVPTX, there's nothing else currently possible to do with that target other than emit assembly that goes to ptxas.
But yeah, as a developer/testing-only feature I don't object too much to a use-ptxas-format flag to cover all this, but I still think it opens up a "wait, is there some other way/reason to be using ptxas other than for the NVPTX target?".

[probinson]

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.

I think the only reason we have a flag for the debugger tuning is that it can't always be deduced from the triple (eg: folks using LLDB on Linux, etc) - but this case can, it's always NVPTX, there's nothing else currently possible to do with that target other than emit assembly that goes to ptxas.
But yeah, as a developer/testing-only feature I don't object too much to a use-ptxas-format flag to cover all this, but I still think it opens up a "wait, is there some other way/reason to be using ptxas other than for the NVPTX target?".

I'd hope that a "use-ptxas-format" cl::opt would have commentary to answer that exact question.

[dblaikie]

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.

Do you trim the target set you build/dev with locally? Does that save much time - I build with the default, all the (non-experimental) targets enabled, so NVPTX failures do happen locally.

I think the only reason we have a flag for the debugger tuning is that it can't always be deduced from the triple (eg: folks using LLDB on Linux, etc) - but this case can, it's always NVPTX, there's nothing else currently possible to do with that target other than emit assembly that goes to ptxas.
But yeah, as a developer/testing-only feature I don't object too much to a use-ptxas-format flag to cover all this, but I still think it opens up a "wait, is there some other way/reason to be using ptxas other than for the NVPTX target?".

I'd hope that a "use-ptxas-format" cl::opt would have commentary to answer that exact question.

Sure enough

[probinson]

In D55281#1334789, @dblaikie wrote:

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.

Do you trim the target set you build/dev with locally? Does that save much time - I build with the default, all the (non-experimental) targets enabled, so NVPTX failures do happen locally.

It saves some. I'm building on a 6-core Windows box so not a huge level of parallelism--one of the prices of working remotely.

lib/CodeGen/AsmPrinter/DwarfFile.cpp

void DwarfFile::emitUnit(DwarfUnit *TheU, bool UseOffsets) {
 
   DIE &Die = TheU->getUnitDie();
   MCSection *USection = TheU->getSection();
   Asm->OutStreamer->SwitchSection(USection);
 
   TheU->emitHeader(UseOffsets);
 
   Asm->emitDwarfDIE(Die);
+
+  Asm->OutStreamer->EmitLabel(TheU->getEndLabel());
 }
 
 // Compute the size and offset for each DIE.
 void DwarfFile::computeSizeAndOffsets() {
   // Offset from the first CU in the debug info section is 0 initially.
   unsigned SecOffset = 0;
 
   // Iterate over each compile unit and set the size and offsets for each

lib/CodeGen/AsmPrinter/DwarfUnit.h

protected:
   /// MDNode for the compile unit.
   const DICompileUnit *CUNode;
 
   // All DIEValues are allocated through this allocator.
   BumpPtrAllocator DIEValueAllocator;
 
   /// Target of Dwarf emission.
   AsmPrinter *Asm;
+  MCSymbol *EndLabel;
 
   // Holders for some common dwarf information.
   DwarfDebug *DD;
   DwarfFile *DU;
 
   /// An anonymous type for index type.  Owned by DIEUnit.
   DIE *IndexTyDie;
 
protected:
   bool applySubprogramDefinitionAttributes(const DISubprogram *SP, DIE &SPDie);
 
   bool shareAcrossDWOCUs() const;
   bool isShareableAcrossCUs(const DINode *D) const;
 
 public:
   // Accessors.
   AsmPrinter* getAsmPrinter() const { return Asm; }
+  MCSymbol *getEndLabel() const { return EndLabel; }
   uint16_t getLanguage() const { return CUNode->getSourceLanguage(); }
   const DICompileUnit *getCUNode() const { return CUNode; }
 
   uint16_t getDwarfVersion() const { return DD->getDwarfVersion(); }
 
   /// Return true if this compile unit has something to write out.
   bool hasContent() const { return getUnitDie().hasChildren(); }
 

lib/CodeGen/AsmPrinter/DwarfUnit.cpp

addUInt(StaticMemberDIE, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,
             AlignInBytes);
 
   return &StaticMemberDIE;
 }
 
 void DwarfUnit::emitCommonHeader(bool UseOffsets, dwarf::UnitType UT) {
   // Emit size of content not including length itself
   Asm->OutStreamer->AddComment("Length of Unit");
-  Asm->emitInt32(getHeaderSize() + getUnitDie().getSize());
+  StringRef Prefix = isDwoUnit() ? "debug_info_dwo_" : "debug_info_";
+  MCSymbol *BeginLabel = Asm->createTempSymbol(Prefix + "start");
+  EndLabel = Asm->createTempSymbol(Prefix + "end");
+  Asm->EmitLabelDifference(EndLabel, BeginLabel, 4);
+  Asm->OutStreamer->EmitLabel(BeginLabel);
 
   Asm->OutStreamer->AddComment("DWARF version number");
   unsigned Version = DD->getDwarfVersion();
   Asm->emitInt16(Version);
 
   // DWARF v5 reorders the address size and adds a unit type.
   if (Version >= 5) {
     Asm->OutStreamer->AddComment("DWARF Unit Type");