[PATCH] D29813: [DAGCombiner] Fix DebugLoc propagation when folding !(x cc y) -> (x !cc y)

Andrea Di Biagio via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 22 04:44:46 PST 2017


andreadb added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4700-4703
+        return DAG.getSetCC(SDLoc(N0), VT, LHS, RHS, NotCC);
       case ISD::SELECT_CC:
-        return DAG.getSelectCC(SDLoc(N), LHS, RHS, N0.getOperand(2),
+        return DAG.getSelectCC(SDLoc(N0), LHS, RHS, N0.getOperand(2),
                                N0.getOperand(3), NotCC);
----------------
twoh wrote:
> andreadb wrote:
> > With this change, we always propagate the debug location of N0.
> > 
> > However, this may be suboptimal if we end up in a situation where N0 has a null DebugLoc, and the DebugLoc of N is not null. In that case, shouldn't we propagate SDLoc(N) instead?
> > 
> > When constructing a new SetCC/SelectCC node, what if we give higher priority to SDLoc(N) over SDLoc(N0) based on whether `N0->getDebugLoc()` returns a null location?
> > 
> > In order to guarantee a deterministic schedule, we could always call `SelectionDAG::UpdadeSDLocOnMergedSDNode()` on the newly created dag node, with the goal to set the new IROrder equal to the minimum node order between N0 and N.
> > 
> > Not sure if that makes sense.
> > I hope this helps.
> @andreadb Thanks for the comments! In this particular case I think it is not correct to take SDLoc from N, and it is better to let the new SDLoc have null DebugLoc if N0 has a null DebugLoc. If we can't have a precise DebugLoc, I think it is better with no DebugLoc than imprecise DebugLoc. 
> 
> Regarding scheduling order, It seems more natural to me that the new SDValue takes IROrder from N0 if it takes SDLoc from N0. A deterministic schedule is a desirable property, but not sure if it needs to be forced here. 
I have to admit, it is not particularly obvious to me why a null DebugLoc is better than a potentially imprecise DebugLoc from N. That said, I am not worried about that particular case.
I think that overall the patch is good - but please wait for Adrian.




https://reviews.llvm.org/D29813





More information about the llvm-commits mailing list