We’ve recently encountered a problem in our compiler where the line number in debug info jumps back and force even at O0. This is caused by DAG node ordering not being properly kept during legalization and instruction selection. There are still uncaught cases after applying the patch mentioned here.
So I have decided to implement the approach suggested by Andy as below. i.e. maintain the node ordering as a field inside the DAG node and force anyone creating the DAG node to provide the ordering. When new DAG nodes are created inside DAG builder, DAG builder maintains the ordering of the current instruction being processed and provide that ordering to the DAG node creating routine. When new DAG nodes are created after DAG builder, e.g., during legalization, the original node’s ordering would be transferred to the new node.
Since it’s going to involve a lot of changes, I’d like to get feedback on the idea and the interface changes before I make changes to all the call sites.
Attached is a diff of the first batch of changes, which includes interface changes: a new wrapper class, new fields, interface changes to SelectionDAG::getXXX() functions.
Thanks for doing this! I think it’s critical, not just for debug information, but for the many people who have started using pre-RA-sched=source. This looks more efficient and maintainable to me than the previous approach.
Preserving the order for everything except a certain class of nodes (e.g. constants) make DAG serialization straightforward. It just needs to insert copies in rare cases, but won’t need to find an optimal order. I’m particularly happy that we can remove uncertainty from the IR->MI conversion. It will be much easier to debug and write tests.
My only concern is merge conflicts in out-of-tree ISelLowering and ISelDAGToDAG. I personally don’t think it will be too bad, since, as Nadav suggested, it’s just a mechanical process of converting DebugLoc definitions:
DebugLoc dl = N->getDebugLoc()
→
SDLoc dl(N);
The getMachineNode call sites themselves should rarely need to change.
Also, we just had a significant API change in this area less than 2 weeks ago. It’s a good time to make this change and do it right once and for all.
But it will be a giant diff. If you think that the changes will be too much, or if anyone else is concerned about merge conflicts, we can consider supporting a backward compatible API.
We’ve recently encountered a problem in our compiler where the line number
in debug info jumps back and force even at O0. This is caused by DAG node
ordering not being properly kept during legalization and instruction
selection. There are still uncaught cases after applying the patch
mentioned here.****
**
Another option is checking how much, at O0, the fast instruction selector
is falling back to the DAG - if you have one at least. Fast isel doesn't
reorder to the degree that selection on the DAG does.
Sorry I wasn’t clear. The problem happened in the “source” pre-RA scheduler, which relies on DAG node ordering to schedule the nodes.
In your case, Eric's suggestion was effectively "start implementing fast-isel". Which is not a bad idea if you want to reduce compile time and improve debug info.
I just really want pre-RA-sched=source to work as advertised.
Sorry I wasn’t clear. The problem happened in the “source” pre-RA
scheduler, which relies on DAG node ordering to schedule the nodes.
In your case, Eric's suggestion was effectively "start implementing
fast-isel". Which is not a bad idea if you want to reduce compile time and
improve debug info.
I just really want pre-RA-sched=source to work as advertised.
We have been hit by this to varying degrees in the past. The hard part a
lot of the time is just tracing through the SDAG transforms and finding
where the ordering information is being lost. +1 for a simple, consistent,
well-maintained API.
The changes I implemented earlier were intended to minimize API churn to
LLVM to make it easier to merge. I would love to see a more "finished"
solution, especially one that considers chains of operations being
generated during legalization!