Teach the SDag type legalizer how to split up debug info for integer values that are split into a hi and lo part.
This is yet another a split-off from http://reviews.llvm.org/D2680
[I messed up the subsribers list previously. Reposting.]
Details
Details
- Reviewers
- None
Diff Detail
Diff Detail
Event Timeline
| lib/CodeGen/SelectionDAG/InstrEmitter.cpp | ||
|---|---|---|
| 675 ↗ | (On Diff #12314) | Not knowing much about any of this SelDAG code, I don't understand how this change is related to the rest of the patch - could you explain it? |
| test/DebugInfo/X86/sdagsplit-1.ll | ||
| 2 | Not sure about your preference, but I usually avoid the temporary file and just pipe from one to the next: ; RUN: %llc_dwarf -filetype=obj < %s | FileCheck %s Note also the use of %llc_dwarf (I keep forgetting why we need that, and why it's not done more consistently - but something to do with Windows support... Takumi knows more about this) and < %s, so that the output doesn't risk containing the filename of the input (which might vary depending on test configuration/directory hierarchy, and thus fail on some machines and pass on others). | |
| 3 | Why the custom check prefix rather than just the default CHECK? | |
| 11 | Any particular reason this needs 3 parameters and a return value? (are they testing different codepaths? If they're all testing the same one, could we just use a single parameter?) | |
| 16 | I see the return value is the only difference between these two test cases. Is it relevant? (I assume so, - guess that's how you ended up with two test cases) Could it be documented in the test better? Is the functionality that makes this test pass separate from the functionality that makes the first test pass - if so, perhaps it could be separated further. I'm still muddling through the actual source changes & trying to figure out how they're all dependent/related. | |
| 36 | I'm a little confused as to how the use of %add comes before its definition... - but I guess that's working as intended somehow? | |
| test/DebugInfo/X86/sdagsplit-2.ll | ||
| 1 ↗ | (On Diff #12314) | Same feedback as the other test case's RUN line. |
Comment Actions
Thanks for the feedback, this is a all-around simplified revision of the previous patch! Particularly, I
- got rid of (now) redundant code in InstrEmitter.
- got rid of the second testcase, which could be reduced to the first without loss of generality.
- simplified the first testcase
| test/DebugInfo/X86/sdagsplit-1.ll | ||
|---|---|---|
| 10 | Some comments about what these bits of code are trying to demonstrate would be helpful. Sometimes I find test cases easier to understand by using stubs to indicate which pieces I care about and why I care about them. (eg: if I need a variable to be 'used' I just have a sink function declared: "void sink(long long)" and call that - helps make it clear that I just want the variable to be used, not that I'm particularly interested in op== comparison, say) In any case, it'd be helpful to understand what things this code is meant to demonstrate - probably apparent in the test case either by comment or self-documenting techniques like I described. Given that you're just testing the location of 'res' - is there any reason you need the a and b parameters at all? | |
| 19 | what's the parameter to piece (the 0x00000004)? It might be helpful for this comment to use the DW_OP names and name the parameters here to make the expression a bit more explicit? (I'm picturing something like what, I assume, Apple's dwarfdump does to print DWARF expressions) | |
Comment Actions
As of now, for targets that do multiple-levels of splitting to legalise variables this patch isn't functional.
[e.g. enable debug on MSP430 (patch available if you want it) and try something like]
long lnot (long x) {
return ~x;
}c.f.
long long qnot (long long x) {
return ~x;
}for the first case, the variable is correctly split into pieces.
for the second it is not.
I've traced a bit through the legaliser code and ISTM that the reason for this is that the legalisation is done leaf-wise:
PAIRA (p0, p1)
/
VAR => PAIR
\
PAIRB (p2, p3)when p0,p1 are visited, the debug information is not present on the PAIRA - since that is only filled in when it is visited (subsequently, as we move from leaves leftwards).
Function arguments are not handled at all by this process (they are split elsewhere, for the present I am assuming that this is intended to be handled by a separate patch?)
| test/DebugInfo/X86/sdagsplit-1.ll | ||
|---|---|---|
| 10 | The original intention of the testcase was to demonstrate piece-wise splitting of the arguments as well as the temporary. As of now the arguments are not handled (will comment on that separately) in fact something as simple as: long long flip ( long long x) {
return ~x;
}might be enough to show the required behaviour, | |
Not sure about your preference, but I usually avoid the temporary file and just pipe from one to the next:
; RUN: %llc_dwarf -filetype=obj < %s | FileCheck %s
Note also the use of %llc_dwarf (I keep forgetting why we need that, and why it's not done more consistently - but something to do with Windows support... Takumi knows more about this) and < %s, so that the output doesn't risk containing the filename of the input (which might vary depending on test configuration/directory hierarchy, and thus fail on some machines and pass on others).