This commit adds a test for debug info generated when the machine
function splitter pass is enabled. We check that the line numbers for
the hot and cold parts of the split function are what we expect them to
be.
Details
Details
- Reviewers
saugustine tmsriram
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
I can't speak to its technical correctness, but this is exactly the test I requested and it looks good at a high level.
Comment Actions
In addition, I wonder where this is layering issue for the testing. This patch adds llvm-objdump -l which is not common in test/DebugInfo. In fact, there is only one test using -S/--source/-l/--line-numbers, so I wonder whether it is the most appropriate testing approach here.
Perhaps this patch should test .loc instead.
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 3 | This doesn't sound right - we can run ELF tests on non-ELF platforms. (I assume that's what's trying to be avoided/addressed by this?) Note all the other tests in this directory that don't have such UNSUPPORTED tagging. | |
| 49 | If this is a general discussion of "What debug info related things should we test for bb-sections in the LLVM project" it'd be good to get a quick summary of what testing already exists. But otherwise, broad testing I'd think:
But maybe both of these tests are already checked in/implemented? I don't think there's much need for Split DWARF-specific testing, as the Split DWARF handling of ranges is fairly orthogonal to where they appear (shouldn't be interesting to Split DWARF that the ranges appear on a DW_TAG_subprogram, I don't think). | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Yes the llvm-dwarfdump and the split dwarf testing of DW_AT_ranges is already there: test/DebugInfo/X86/basic-block-sections_1.ll | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Could the coverage this new test is adding be added to the existing test? Looks like this new test is intended to cover the line table - which could be added to expanded coverage in the basic-block-sections_1.ll by dumping debug_line as well as debug_info (adding "-debug-line" to those existing dwarfdump commands) | |
Comment Actions
Updates based on review comments.
- Use llvm-dwarfdump --debug-line instead of llvm-objdump -l
- Updated the checks, added line numbers to the source.
- Removed unsupported tag, should no longer be required since we don't use llvm-objdump anymore.
Comment Actions
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 3 | Removed, I think this was failing on windows due to the use of llvm-objdump. I'll keep an eye on the windows bot and double check. | |
| 31 | The cold block is the one below, the modulus yields the remainder which is interpreted as true for the if condition. | |
| 49 | I think there is value in having a simple test for the line table which many downstream tools rely on. We should add a line table test for basic block sections too. | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Generally we'd test all the things related to a feature/new/interesting output in the same place. Sometimes worth splitting into multiple files, but there's some benefits to keeping it all together (can page in the context for one feature area, less test process overhead, etc). Admittedly, the tests aren't especially rigorously laid out by any means. And I see I got this jumbled up between split-machine-functions and bb-sections. What's the difference between those two features? I /guess/ that split-machine-functions is a specific application of/builds on top of bb-sections? | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Yes, split-machine-functions is a specific application of basic block sections. The key difference here is the type of profile consumed to drive the layout decisions (FDO/AFDO vs Propeller). From the perspective of the machine function splitting pass this test ensures that the underlying mechanism, i.e. basic block sections updates debug information correctly. Thus I added an independent test. There is some duplication here and if you feel strongly we can just test the line table in the existing basic block sections. WDYT? | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Yeah, seems like it's probably better to test this functionality down at the bb-sections level, since that's the functionality it's really testing/relying on. If several features are built on top of bb-sections, we don't want the fundamental testing to only be done via one of those features (if we then remove that feature - might lose the testing), and makes it clearer what testing covers what functionality/implementation code. Looks like the debug info functionality being tested isn't specific to split-machine-functions, so I'd generally favor not testing it here and instead testing it with bb-sections. (if there's specific DWARF code/functionality only reachable through split-machine-functions, that should be tested here) | |
| llvm/test/DebugInfo/X86/machine-function-splitter.ll | ||
|---|---|---|
| 49 | Sent out https://reviews.llvm.org/D90989 to check the line table for basic block sections. | |
This doesn't sound right - we can run ELF tests on non-ELF platforms. (I assume that's what's trying to be avoided/addressed by this?) Note all the other tests in this directory that don't have such UNSUPPORTED tagging.