<font face="Verdana,Arial,Helvetica,sans-serif" size="2">Hi, Owen,<br><br>Do you have any test/IR for me so that I can measure the compiling time impact. For now, isReachable() is basing on the dynamic DAG which can be improved to static DAG with the loss of some corner cases.<span> I will have some try to see if it fixes your problem.<br><br>Best regards<br><br>steven.zhang(张青山)<br>XL/LLVM on Power Compiler Developer<br><br>IBM China Development Lab, Shanghai <br>Tel: (8621)609-28454 Mobile: +8615900986116<br>E-mail: <a href="mailto:qshanz@cn.ibm.com">qshanz@cn.ibm.com</a></span><br><br><font size="2" face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" color="#000000"><font color="#990099">-----Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>> wrote: -----</font><div class="iNotesHistory" style="padding-left:5px;"><div style="padding-right:0px;padding-left:5px;border-left:solid black 2px;">To: QingShan Zhang <<a href="mailto:qshanz@cn.ibm.com" target="_blank">qshanz@cn.ibm.com</a>>, QingShan Zhang <<a href="mailto:llvmlistbot@llvm.org" target="_blank">llvmlistbot@llvm.org</a>><br>From: Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>><br>Date: 10/23/2020 02:28AM<br>Cc: <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>Subject: [EXTERNAL] Re: [llvm] ebf3b18 - [Scheduling] Implement a new way to cluster loads/stores<br><br><div><font size="2" face="Courier New,Courier,monospace">Hi QingShan,<br><br>I’m seeing significant compile time problems introduced by this change on AArch64, with a worst case example taking >10 minutes to compile.  The root cause seems to be the IsReachable() calls introduced here - they’re not cheap, and they’re being called a lot.  Is there some way we can back off or limit the number of queries here?<br><br>—Owen<br><br>> On Aug 26, 2020, at 5:34 AM, QingShan Zhang via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>> <br>> <br>> Author: QingShan Zhang<br>> Date: 2020-08-26T12:33:59Z<br>> New Revision: ebf3b188c6edcce7e90ddcacbe7c51c90d95b0ac<br>> <br>> URL: <a href="https://github.com/llvm/llvm-project/commit/ebf3b188c6edcce7e90ddcacbe7c51c90d95b0ac">https://github.com/llvm/llvm-project/commit/ebf3b188c6edcce7e90ddcacbe7c51c90d95b0ac</a> <br>> DIFF: <a href="https://github.com/llvm/llvm-project/commit/ebf3b188c6edcce7e90ddcacbe7c51c90d95b0ac.diff">https://github.com/llvm/llvm-project/commit/ebf3b188c6edcce7e90ddcacbe7c51c90d95b0ac.diff</a> <br>> <br>> LOG: [Scheduling] Implement a new way to cluster loads/stores<br>> <br>> Before calling target hook to determine if two loads/stores are clusterable,<br>> we put them into different groups to avoid fake cluster due to dependency.<br>> For now, we are putting the loads/stores into the same group if they have<br>> the same predecessor. We assume that, if two loads/stores have the same<br>> predecessor, it is likely that, they didn't have dependency for each other.<br>> <br>> However, one SUnit might have several predecessors and for now, we just<br>> pick up the first predecessor that has non-data/non-artificial dependency,<br>> which is too arbitrary. And we are struggling to fix it.<br>> <br>> So, I am proposing some better implementation.<br>> 1. Collect all the loads/stores that has memory info first to reduce the complexity.<br>> 2. Sort these loads/stores so that we can stop the seeking as early as possible.<br>> 3. For each load/store, seeking for the first non-dependency instruction with the<br>>  sorted order, and check if they can cluster or not.<br>> <br>> Reviewed By: Jay Foad<br>> <br>> Differential Revision: <a href="https://reviews.llvm.org/D85517">https://reviews.llvm.org/D85517</a> <br>> <br>> Added: <br>> <br>> <br>> Modified: <br>>   llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h<br>>   llvm/lib/CodeGen/MachineScheduler.cpp<br>>   llvm/test/CodeGen/AArch64/aarch64-stp-cluster.ll<br>>   llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll<br>>   llvm/test/CodeGen/AMDGPU/max.i16.ll<br>>   llvm/test/CodeGen/AMDGPU/stack-realign.ll<br>> <br>> Removed: <br>> <br>> <br>> <br>> ################################################################################<br>> diff  --git a/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h b/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h<br>> index 1eb9b9f322ba..d2b95209d7b4 100644<br>> --- a/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h<br>> +++ b/llvm/include/llvm/CodeGen/ScheduleDAGInstrs.h<br>> @@ -268,6 +268,11 @@ namespace llvm {<br>>      return SU->SchedClass;<br>>    }<br>> <br>> +    /// IsReachable - Checks if SU is reachable from TargetSU.<br>> +    bool IsReachable(SUnit *SU, SUnit *TargetSU) {<br>> +      return Topo.IsReachable(SU, TargetSU);<br>> +    }<br>> +<br>>    /// Returns an iterator to the top of the current scheduling region.<br>>    MachineBasicBlock::iterator begin() const { return RegionBegin; }<br>> <br>> <br>> diff  --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp<br>> index a8ccf2643f20..b6d0d9a74ac1 100644<br>> --- a/llvm/lib/CodeGen/MachineScheduler.cpp<br>> +++ b/llvm/lib/CodeGen/MachineScheduler.cpp<br>> @@ -1530,7 +1530,10 @@ class BaseMemOpClusterMutation : public ScheduleDAGMutation {<br>>  void apply(ScheduleDAGInstrs *DAGInstrs) override;<br>> <br>> protected:<br>> -  void clusterNeighboringMemOps(ArrayRef<SUnit *> MemOps, ScheduleDAGInstrs *DAG);<br>> +  void clusterNeighboringMemOps(ArrayRef<MemOpInfo> MemOps,<br>> +                                ScheduleDAGInstrs *DAG);<br>> +  void collectMemOpRecords(std::vector<SUnit> &SUnits,<br>> +                           SmallVectorImpl<MemOpInfo> &MemOpRecords);<br>> };<br>> <br>> class StoreClusterMutation : public BaseMemOpClusterMutation {<br>> @@ -1566,63 +1569,53 @@ createStoreClusterDAGMutation(const TargetInstrInfo *TII,<br>> <br>> } // end namespace llvm<br>> <br>> +// Sorting all the loads/stores first, then for each load/store, checking the<br>> +// following load/store one by one, until reach the first non-dependent one and<br>> +// call target hook to see if they can cluster.<br>> void BaseMemOpClusterMutation::clusterNeighboringMemOps(<br>> -    ArrayRef<SUnit *> MemOps, ScheduleDAGInstrs *DAG) {<br>> -  SmallVector<MemOpInfo, 32> MemOpRecords;<br>> -  for (SUnit *SU : MemOps) {<br>> -    const MachineInstr &MI = *SU->getInstr();<br>> -    SmallVector<const MachineOperand *, 4> BaseOps;<br>> -    int64_t Offset;<br>> -    bool OffsetIsScalable;<br>> -    unsigned Width;<br>> -    if (TII->getMemOperandsWithOffsetWidth(MI, BaseOps, Offset,<br>> -                                           OffsetIsScalable, Width, TRI)) {<br>> -      MemOpRecords.push_back(MemOpInfo(SU, BaseOps, Offset, Width));<br>> -<br>> -      LLVM_DEBUG(dbgs() << "Num BaseOps: " << BaseOps.size() << ", Offset: "<br>> -                        << Offset << ", OffsetIsScalable: " << OffsetIsScalable<br>> -                        << ", Width: " << Width << "\n");<br>> -    }<br>> -#ifndef NDEBUG<br>> -    for (auto *Op : BaseOps)<br>> -      assert(Op);<br>> -#endif<br>> -  }<br>> -  if (MemOpRecords.size() < 2)<br>> -    return;<br>> -<br>> -  llvm::sort(MemOpRecords);<br>> +    ArrayRef<MemOpInfo> MemOpRecords, ScheduleDAGInstrs *DAG) {<br>> +  // Keep track of the current cluster length and bytes for each SUnit.<br>> +  DenseMap<unsigned, std::pair<unsigned, unsigned>> SUnit2ClusterInfo;<br>> <br>>  // At this point, `MemOpRecords` array must hold atleast two mem ops. Try to<br>>  // cluster mem ops collected within `MemOpRecords` array.<br>> -  unsigned ClusterLength = 1;<br>> -  unsigned CurrentClusterBytes = MemOpRecords[0].Width;<br>>  for (unsigned Idx = 0, End = MemOpRecords.size(); Idx < (End - 1); ++Idx) {<br>>    // Decision to cluster mem ops is taken based on target dependent logic<br>>    auto MemOpa = MemOpRecords[Idx];<br>> -    auto MemOpb = MemOpRecords[Idx + 1];<br>> -    ++ClusterLength;<br>> -    CurrentClusterBytes += MemOpb.Width;<br>> -    if (!TII->shouldClusterMemOps(MemOpa.BaseOps, MemOpb.BaseOps, ClusterLength,<br>> -                                  CurrentClusterBytes)) {<br>> -      // Current mem ops pair could not be clustered, reset cluster length, and<br>> -      // go to next pair<br>> -      ClusterLength = 1;<br>> -      CurrentClusterBytes = MemOpb.Width;<br>> +<br>> +    // Seek for the next load/store to do the cluster.<br>> +    unsigned NextIdx = Idx + 1;<br>> +    for (; NextIdx < End; ++NextIdx)<br>> +      // Skip if MemOpb has been clustered already or has dependency with<br>> +      // MemOpa.<br>> +      if (!SUnit2ClusterInfo.count(MemOpRecords[NextIdx].SU->NodeNum) &&<br>> +          !DAG->IsReachable(MemOpRecords[NextIdx].SU, MemOpa.SU) &&<br>> +          !DAG->IsReachable(MemOpa.SU, MemOpRecords[NextIdx].SU))<br>> +        break;<br>> +    if (NextIdx == End)<br>>      continue;<br>> +<br>> +    auto MemOpb = MemOpRecords[NextIdx];<br>> +    unsigned ClusterLength = 2;<br>> +    unsigned CurrentClusterBytes = MemOpa.Width + MemOpb.Width;<br>> +    if (SUnit2ClusterInfo.count(MemOpa.SU->NodeNum)) {<br>> +      ClusterLength = SUnit2ClusterInfo[MemOpa.SU->NodeNum].first + 1;<br>> +      CurrentClusterBytes =<br>> +          SUnit2ClusterInfo[MemOpa.SU->NodeNum].second + MemOpb.Width;<br>>    }<br>> <br>> +    if (!TII->shouldClusterMemOps(MemOpa.BaseOps, MemOpb.BaseOps, ClusterLength,<br>> +                                  CurrentClusterBytes))<br>> +      continue;<br>> +<br>>    SUnit *SUa = MemOpa.SU;<br>>    SUnit *SUb = MemOpb.SU;<br>>    if (SUa->NodeNum > SUb->NodeNum)<br>>      std::swap(SUa, SUb);<br>> <br>>    // FIXME: Is this check really required?<br>> -    if (!DAG->addEdge(SUb, SDep(SUa, SDep::Cluster))) {<br>> -      ClusterLength = 1;<br>> -      CurrentClusterBytes = MemOpb.Width;<br>> +    if (!DAG->addEdge(SUb, SDep(SUa, SDep::Cluster)))<br>>      continue;<br>> -    }<br>> <br>>    LLVM_DEBUG(dbgs() << "Cluster ld/st SU(" << SUa->NodeNum << ") - SU("<br>>                      << SUb->NodeNum << ")\n");<br>> @@ -1656,42 +1649,57 @@ void BaseMemOpClusterMutation::clusterNeighboringMemOps(<br>>      }<br>>    }<br>> <br>> +    SUnit2ClusterInfo[MemOpb.SU->NodeNum] = {ClusterLength,<br>> +                                             CurrentClusterBytes};<br>> +<br>>    LLVM_DEBUG(dbgs() << "  Curr cluster length: " << ClusterLength<br>>                      << ", Curr cluster bytes: " << CurrentClusterBytes<br>>                      << "\n");<br>>  }<br>> }<br>> <br>> -/// Callback from DAG postProcessing to create cluster edges for loads.<br>> -void BaseMemOpClusterMutation::apply(ScheduleDAGInstrs *DAG) {<br>> -  // Map DAG NodeNum to a set of dependent MemOps in store chain.<br>> -  DenseMap<unsigned, SmallVector<SUnit *, 4>> StoreChains;<br>> -  for (SUnit &SU : DAG->SUnits) {<br>> +void BaseMemOpClusterMutation::collectMemOpRecords(<br>> +    std::vector<SUnit> &SUnits, SmallVectorImpl<MemOpInfo> &MemOpRecords) {<br>> +  for (auto &SU : SUnits) {<br>>    if ((IsLoad && !SU.getInstr()->mayLoad()) ||<br>>        (!IsLoad && !SU.getInstr()->mayStore()))<br>>      continue;<br>> <br>> -    unsigned ChainPredID = DAG->SUnits.size();<br>> -    for (const SDep &Pred : SU.Preds) {<br>> -      // We only want to cluster the mem ops that have the same ctrl(non-data)<br>> -      // pred so that they didn't have ctrl dependency for each other. But for<br>> -      // store instrs, we can still cluster them if the pred is load instr.<br>> -      if ((Pred.isCtrl() &&<br>> -           (IsLoad ||<br>> -            (Pred.getSUnit() && Pred.getSUnit()->getInstr()->mayStore()))) &&<br>> -          !Pred.isArtificial()) {<br>> -        ChainPredID = Pred.getSUnit()->NodeNum;<br>> -        break;<br>> -      }<br>> +    const MachineInstr &MI = *SU.getInstr();<br>> +    SmallVector<const MachineOperand *, 4> BaseOps;<br>> +    int64_t Offset;<br>> +    bool OffsetIsScalable;<br>> +    unsigned Width;<br>> +    if (TII->getMemOperandsWithOffsetWidth(MI, BaseOps, Offset,<br>> +                                           OffsetIsScalable, Width, TRI)) {<br>> +      MemOpRecords.push_back(MemOpInfo(&SU, BaseOps, Offset, Width));<br>> +<br>> +      LLVM_DEBUG(dbgs() << "Num BaseOps: " << BaseOps.size() << ", Offset: "<br>> +                        << Offset << ", OffsetIsScalable: " << OffsetIsScalable<br>> +                        << ", Width: " << Width << "\n");<br>>    }<br>> -    // Insert the SU to corresponding store chain.<br>> -    auto &Chain = StoreChains.FindAndConstruct(ChainPredID).second;<br>> -    Chain.push_back(&SU);<br>> +#ifndef NDEBUG<br>> +    for (auto *Op : BaseOps)<br>> +      assert(Op);<br>> +#endif<br>>  }<br>> +}<br>> +<br>> +/// Callback from DAG postProcessing to create cluster edges for loads/stores.<br>> +void BaseMemOpClusterMutation::apply(ScheduleDAGInstrs *DAG) {<br>> +  // Collect all the clusterable loads/stores<br>> +  SmallVector<MemOpInfo, 32> MemOpRecords;<br>> +  collectMemOpRecords(DAG->SUnits, MemOpRecords);<br>> +<br>> +  if (MemOpRecords.size() < 2)<br>> +    return;<br>> +<br>> +  // Sorting the loads/stores, so that, we can stop the cluster as early as<br>> +  // possible.<br>> +  llvm::sort(MemOpRecords);<br>> <br>> -  // Iterate over the store chains.<br>> -  for (auto &SCD : StoreChains)<br>> -    clusterNeighboringMemOps(SCD.second, DAG);<br>> +  // Trying to cluster all the neighboring loads/stores.<br>> +  clusterNeighboringMemOps(MemOpRecords, DAG);<br>> }<br>> <br>> //===----------------------------------------------------------------------===//<br>> <br>> diff  --git a/llvm/test/CodeGen/AArch64/aarch64-stp-cluster.ll b/llvm/test/CodeGen/AArch64/aarch64-stp-cluster.ll<br>> index b0ed3d0490cc..e95321582def 100644<br>> --- a/llvm/test/CodeGen/AArch64/aarch64-stp-cluster.ll<br>> +++ b/llvm/test/CodeGen/AArch64/aarch64-stp-cluster.ll<br>> @@ -214,11 +214,11 @@ entry:<br>>  ret void<br>> }<br>> <br>> -; FIXME - The SU(4) and SU(7) can be clustered even with<br>> +; Verify that the SU(4) and SU(7) can be clustered even with<br>> ; <br>> diff erent preds<br>> ; CHECK: ********** MI Scheduling **********<br>> ; CHECK-LABEL: cluster_with_<br>> diff erent_preds:%bb.0<br>> -; CHECK-NOT:Cluster ld/st SU(4) - SU(7)<br>> +; CHECK:Cluster ld/st SU(4) - SU(7)<br>> ; CHECK:SU(3):   STRWui %2:gpr32, %0:gpr64common, 0 ::<br>> ; CHECK:SU(4):   %3:gpr32 = LDRWui %1:gpr64common, 0 ::<br>> ; CHECK:Predecessors:<br>> <br>> diff  --git a/llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll b/llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll<br>> index 2dc47ca94aa9..d23538cadcbd 100644<br>> --- a/llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll<br>> +++ b/llvm/test/CodeGen/AMDGPU/callee-special-input-vgprs.ll<br>> @@ -624,9 +624,9 @@ define void @too_many_args_use_workitem_id_x_byval(<br>> <br>> <br>> ; FIXEDABI: v_mov_b32_e32 [[K0:v[0-9]+]], 0x3e7<br>> +; FIXEDABI: buffer_store_dword [[K0]], off, s[0:3], 0 offset:4{{$}}<br>> ; FIXEDABI: s_movk_i32 s32, 0x400{{$}}<br>> ; FIXEDABI: v_mov_b32_e32 [[K1:v[0-9]+]], 0x140<br>> -; FIXEDABI: buffer_store_dword [[K0]], off, s[0:3], 0 offset:4{{$}}<br>> <br>> ; FIXEDABI: buffer_store_dword [[K1]], off, s[0:3], s32{{$}}<br>> <br>> @@ -669,8 +669,8 @@ define amdgpu_kernel void @kern_call_too_many_args_use_workitem_id_x_byval() #1<br>> <br>> ; FIXED-ABI-NOT: v31<br>> ; FIXEDABI: v_mov_b32_e32 [[K0:v[0-9]+]], 0x3e7{{$}}<br>> -; FIXEDABI: v_mov_b32_e32 [[K1:v[0-9]+]], 0x140{{$}}<br>> ; FIXEDABI: buffer_store_dword [[K0]], off, s[0:3], s33{{$}}<br>> +; FIXEDABI: v_mov_b32_e32 [[K1:v[0-9]+]], 0x140{{$}}<br>> ; FIXEDABI: buffer_store_dword [[K1]], off, s[0:3], s32{{$}}<br>> ; FIXEDABI: buffer_load_dword [[RELOAD_BYVAL:v[0-9]+]], off, s[0:3], s33{{$}}<br>> <br>> <br>> diff  --git a/llvm/test/CodeGen/AMDGPU/max.i16.ll b/llvm/test/CodeGen/AMDGPU/max.i16.ll<br>> index c64e400e628f..7c4ce5d6c1ff 100644<br>> --- a/llvm/test/CodeGen/AMDGPU/max.i16.ll<br>> +++ b/llvm/test/CodeGen/AMDGPU/max.i16.ll<br>> @@ -145,14 +145,14 @@ define amdgpu_kernel void @v_test_imax_sge_v3i16(<3 x i16> addrspace(1)* %out, <<br>> ; GFX9-NEXT:    v_mov_b32_e32 v1, 0<br>> ; GFX9-NEXT:    v_mov_b32_e32 v2, 0<br>> ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)<br>> -; GFX9-NEXT:    global_load_short_d16 v2, v0, s[6:7] offset:4<br>> ; GFX9-NEXT:    global_load_short_d16 v1, v0, s[0:1] offset:4<br>> -; GFX9-NEXT:    global_load_dword v3, v0, s[6:7]<br>> -; GFX9-NEXT:    global_load_dword v4, v0, s[0:1]<br>> -; GFX9-NEXT:    s_waitcnt vmcnt(2)<br>> +; GFX9-NEXT:    global_load_dword v3, v0, s[0:1]<br>> +; GFX9-NEXT:    global_load_short_d16 v2, v0, s[6:7] offset:4<br>> +; GFX9-NEXT:    global_load_dword v4, v0, s[6:7]<br>> +; GFX9-NEXT:    s_waitcnt vmcnt(1)<br>> ; GFX9-NEXT:    v_pk_max_i16 v1, v2, v1<br>> ; GFX9-NEXT:    s_waitcnt vmcnt(0)<br>> -; GFX9-NEXT:    v_pk_max_i16 v3, v3, v4<br>> +; GFX9-NEXT:    v_pk_max_i16 v3, v4, v3<br>> ; GFX9-NEXT:    global_store_short v0, v1, s[4:5] offset:4<br>> ; GFX9-NEXT:    global_store_dword v0, v3, s[4:5]<br>> ; GFX9-NEXT:    s_endpgm<br>> <br>> diff  --git a/llvm/test/CodeGen/AMDGPU/stack-realign.ll b/llvm/test/CodeGen/AMDGPU/stack-realign.ll<br>> index 74b53802ef5b..e8e3518aed1c 100644<br>> --- a/llvm/test/CodeGen/AMDGPU/stack-realign.ll<br>> +++ b/llvm/test/CodeGen/AMDGPU/stack-realign.ll<br>> @@ -160,16 +160,14 @@ define void @func_call_align1024_bp_gets_vgpr_spill(<32 x i32> %a, i32 %b) #0 {<br>> ; GCN-NEXT: s_mov_b64 exec, s[4:5]<br>> ; GCN-NEXT: v_writelane_b32 [[VGPR_REG]], s33, 2<br>> ; GCN-NEXT: v_writelane_b32 [[VGPR_REG]], s34, 3<br>> -; GCN: s_mov_b32 s34, s32<br>> ; GCN: s_add_u32 [[SCRATCH_REG:s[0-9]+]], s32, 0xffc0<br>> ; GCN: s_and_b32 s33, [[SCRATCH_REG]], 0xffff0000<br>> +; GCN: s_mov_b32 s34, s32<br>> +; GCN: v_mov_b32_e32 v32, 0<br>> +; GCN: buffer_store_dword v32, off, s[0:3], s33 offset:1024<br>> ; GCN-NEXT: buffer_load_dword v{{[0-9]+}}, off, s[0:3], s34<br>> ; GCN-NEXT: s_add_u32 s32, s32, 0x30000<br>> <br>> -; GCN: v_mov_b32_e32 v33, 0<br>> -<br>> -; GCN: buffer_store_dword v33, off, s[0:3], s33 offset:1024<br>> -<br>> ; GCN: buffer_store_dword v{{[0-9]+}}, off, s[0:3], s32<br>> ; GCN-NEXT: s_swappc_b64 s[30:31], s[4:5]<br>> <br>> <br>> <br>> <br>> _______________________________________________<br>> llvm-commits mailing list<br>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a> <br><br></font></div></div></div></font></font><BR>