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

[Taewook Oh via Phabricator via llvm-commits]

twoh 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);
----------------
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. 


https://reviews.llvm.org/D29813