- User Since
- Sep 26 2016, 7:58 AM (125 w, 5 d)
placeDbgValues has been bugging me (and probably others) for some time. So I'm happy with this patch.
Maybe it is a philosophical question. Is the compiler scheduling/reordering source instructions (in the debugger it will appear as source statements are executed in a random order), or are we scheduling/reordering machine instructions (and in the debugger it still should appear as if we execute variable assignments in the order of the source code). VLIW-scheduling, tail merging, etc, of course make things extra complicated, since we basically will be all over the place all the time.
To me it's the latter, my mental model (maybe wrong) is that while the compiler is optimising code, for debuginfo it has a state-machine of variable locations represented by dbg.values that it needs to try and preserve the order of, marking locations undef if they don't exist any more. The logical consequence is that if we had a function where the compiler managed to completely reverse the order of computation, no variable would have a location, which would be sound, but not useful.
Thu, Feb 21
Wed, Feb 20
Fri, Feb 15
I have some doubt about this. Mainly the part about inserting undefs. Although, some of my doubts are partially based on the big-black-hole regarding how debug info is supposed to work for heavily optimized code (and more specifically how this is supposed to work in LLVM based on what we got today).
Thu, Feb 14
Wed, Feb 13
LGTM! But remember to update the "summary" in the commit msg before commit. It is a little bit out-dated at the moment (e.g. the info about sparc test fixup).
Tue, Feb 12
LGTM as well!
Mon, Feb 11
Sat, Feb 9
Minor adjustments to the test case (added some missing CHECK-NOT).
Updated code comments.
Thu, Feb 7
Wed, Feb 6
Revert the no longer needed modification in test/DebugInfo/Sparc/subreg.ll
Added some code to make test/DebugInfo/AArch64/inlined-argument.ll pass.
I now allow emitting using ArgDbgValue if the dbg.value intrinsic is
in the beginning of the function entry (before any non-dbg instruction).
This is a little bit hacky, but the best solution I could find.
Tue, Feb 5
Mon, Feb 4
There is some problem with this making the test/DebugInfo/AArch64/inlined-argument.ll fail. The inlined "resources" parameter will appear optimized out. I haven't figured out why yet.
Fri, Feb 1
Nice! Me and @dstenb actually discussed this kind of problem earlier today. So quite fun (and surprising) to see a solution pop up in the inbox.
Thu, Jan 31
Wed, Jan 30
Tue, Jan 29
Mon, Jan 28
Added test cases to verify that we still optmize for VLA (as long as all non-variadic arguments are provided).
Please have another look.
- Fix for VarArgs (need to allow more actual than formal arguments).
- Added check for matching argument types (including a new test case).
Jan 23 2019
Jan 22 2019
Jan 21 2019
Should https://reviews.llvm.org/D56987 be a parent for this? Then you'd need to rebase getExpandedFixedPointMultiplication since that has changed into converting into MUL when scale is zero (that is not valid for saturation).
How common would it be that the scale is zero? Is that really expected in reality or just in this kind of handwritten test cases?
Jan 18 2019
Some more updates (some tweaks after having looked at some logs).
Jan 17 2019
Now refactored based on trunk (the first version I uploaded was a
quite old patch that didn't even compile, sorry for that).
Jan 16 2019
@jmorse : Since you are working a little bit with the SelectionDAGBuilder and debug values etc, maybe you will find this patch helpful. It gives some more printouts for the "missing" DBG_VALUE nodes that normally isn't shown during ISel simply by using -debug or -dag-dump-verbose.
PS. I found my old git branch where I played around with this, and I think that I never figured out that I needed the !isa<Argument> check. Maybe that is why I had mixed feeling about the result back in the days.
Jan 15 2019
Jan 14 2019
Interesting. If I remember correctly this is similar to something I played around with half a year ago (as a preparation before prohibiting CodeGenPrepare::placeDbgValues to move DBG_VALUE between basic blocks (or even to disable CodeGenPrepare::placeDbgValues completely)). I'll have a closer look at this when I'm at the office. I'll also try to remember if I had some specific reasons for not moving forward with my attempt at doing this - it has been bothering me that I never found the time to finalize that patch.
Jan 11 2019
Jan 10 2019
Dec 21 2018
Dec 14 2018
Dec 4 2018
Nov 29 2018
Might wait a little before you land this to see if @uweigand got anything more to say. But afaict this only removes the warning without affecting the result in any way, so it should not make anything worse.
Got a feeling that I'm the one who has been questioning this solution. And I guess I also kind of blocked the progress in D54962.
Anyway, I haven't really got any better alternative right now (at least not that is easy to implement).
Nov 28 2018
Nov 27 2018
Nov 22 2018
I don't have any objections to the patch myself, but I don't really have the knowledge to understand if this could be bad for some important use cases either. A user can ofcourse override the threshold if needed, so I guess that makes this patch a little bit less "dangerous".
Nov 21 2018
Nov 20 2018
Basically I think the code looks ok. However, I'm not so familiar with this algorithm so it is hard to comment about the actual solution.
Nov 14 2018
Nov 11 2018
LGTM now (might wanna wait for @skatkov to take a look at the last changes as well).
Nov 10 2018
Maybe the register allocator should add the implicit-def as an IMPLICIT_DEF instruction just before the MI instead of attaching a bogus impl def on the MI (if the MI uses parts of the register I guess the IMPLICIT_DEF needs a corresponding implicit use).