[PATCH] D45995: [DAGCombiner] Set the right SDLoc on a newly-created zextload (1/N)

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8032
 
       ExtendSetCCUses(SetCCs, N0, ExtLoad, SDLoc(N), ISD::ZERO_EXTEND);
       // If the load value is used only by N, replace it via CombineTo N.
----------------
vsk wrote:
> vsk wrote:
> > aprantl wrote:
> > > Out of curiosity, why doesn't this also get the `SDLoc(N0)`?
> > I'm not sure what SetCCUses are. It's possible that these need a different location. I can look into it as a follow-up.
> Looking at this some more, it seems like applying SDLoc(ExtLoad) to the new SetCC nodes would make more sense.
I've proposed a patch for this here: https://reviews.llvm.org/D46216


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7483
       else
         Ops.push_back(DAG.getNode(ExtType, DL, ExtLoad->getValueType(0), SOp));
     }
----------------
niravd wrote:
> As per previous discussion, this should probably be SDLoad(ExtLoad).
See: https://reviews.llvm.org/D46216


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7560
     SDValue SplitLoad = DAG.getExtLoad(
         ExtType, DL, SplitDstVT, LN0->getChain(), BasePtr,
         LN0->getPointerInfo().getWithOffset(Offset), SplitSrcVT, Align,
----------------
niravd wrote:
> Ditto.
For posterity, the patch for this is: https://reviews.llvm.org/D46156


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7786
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, DL, VT, LN0->getChain(),
                                        LN0->getBasePtr(), N0.getValueType(),
----------------
niravd wrote:
> We should be consistant and make sure we choose the load's location for every DAG.getExtLoad.
I'm planning on addressing this here: https://reviews.llvm.org/D46158


https://reviews.llvm.org/D45995