[PATCH] D45995: [DAGCombiner] Set the right SDLoc on a newly-created zextload

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7560
     SDValue SplitLoad = DAG.getExtLoad(
         ExtType, DL, SplitDstVT, LN0->getChain(), BasePtr,
         LN0->getPointerInfo().getWithOffset(Offset), SplitSrcVT, Align,
----------------
Ditto.


================
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(),
----------------
We should be consistant and make sure we choose the load's location for every DAG.getExtLoad.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7817
         TLI.isLoadExtLegal(ISD::SEXTLOAD, VT, MemVT)) {
       SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, DL, VT, LN0->getChain(),
                                        LN0->getBasePtr(), MemVT,
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8192
         TLI.isLoadExtLegal(ISD::ZEXTLOAD, VT, MemVT)) {
       SDValue ExtLoad = DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(N), VT,
                                        LN0->getChain(),
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8342
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::EXTLOAD, SDLoc(N), VT,
                                        LN0->getChain(),
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8371
     if (!LegalOperations || TLI.isLoadExtLegal(ExtType, VT, MemVT)) {
       SDValue ExtLoad = DAG.getExtLoad(ExtType, SDLoc(N),
                                        VT, LN0->getChain(), LN0->getBasePtr(),
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8733
     LoadSDNode *LN0 = cast<LoadSDNode>(N0);
     SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, SDLoc(N), VT,
                                      LN0->getChain(),
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8749
     LoadSDNode *LN0 = cast<LoadSDNode>(N0);
     SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, SDLoc(N), VT,
                                      LN0->getChain(),
----------------
Ditto


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11190
     LoadSDNode *LN0 = cast<LoadSDNode>(N0);
     SDValue ExtLoad = DAG.getExtLoad(ISD::EXTLOAD, SDLoc(N), VT,
                                      LN0->getChain(),
----------------
Ditto


================
Comment at: test/CodeGen/AArch64/arm64-aapcs.ll:31
+; CHECK-LABEL: test_stack_slots:
+; CHECK-DAG: ldr w[[ext1:[0-9]+]], [sp, #24]
+; CHECK-DAG: ldrh w[[ext2:[0-9]+]], [sp, #16]
----------------
vsk wrote:
> aprantl wrote:
> > So the order of these instructions is irrelevant?
> It seems so, although it'd be nice if @ab could chime in about this.
They're definitely unordered. 


================
Comment at: test/CodeGen/X86/avg.ll:296
 ; AVX1-NEXT:    vpmovzxbd {{.*#+}} xmm2 = xmm2[0],zero,zero,zero,xmm2[1],zero,zero,zero,xmm2[2],zero,zero,zero,xmm2[3],zero,zero,zero
-; AVX1-NEXT:    vmovdqa %xmm2, -{{[0-9]+}}(%rsp) # 16-byte Spill
+; AVX1-NEXT:    vmovdqa %xmm2, {{[-0-9]+}}(%r{{[sb]}}p) # 16-byte Spill
 ; AVX1-NEXT:    vpmovzxbd {{.*#+}} xmm2 = xmm5[0],zero,zero,zero,xmm5[1],zero,zero,zero,xmm5[2],zero,zero,zero,xmm5[3],zero,zero,zero
----------------
Much of this test looks like if you ran the update llc test script before this patch they would be generalized and be equivalent. Since you're making revisions, could you land that before updating this patch?


================
Comment at: test/CodeGen/X86/win-smallparams.ll:60
 ; WIN32-LABEL: _manyargs:
-; WIN32-DAG: movsbl 4(%esp),
-; WIN32-DAG: movswl 8(%esp),
-; WIN32-DAG: movzbl 12(%esp),
-; WIN32-DAG: movzwl 16(%esp),
-; WIN32-DAG: movzbl 20(%esp),
-; WIN32-DAG: movzwl 24(%esp),
+; WIN32: pushl %ebx
+; WIN32: pushl %edi
----------------
Were these pushes here before this patch?


https://reviews.llvm.org/D45995