<div dir="ltr">Hi Quentin,<div><br></div><div>Thanks for letting me know the change and the suggestion. You are right that one live interval could have only one segment which covers a lot of basicblocks. I don't remember clearly whether many basicblocks will necessarily mean high compile time cost for global splitting. When I wrote the patch, I had a problematic case to solve which was caused by live interval with many segments. To minimize the impact while still solving the problematic case, I check live interval size -- as long as live interval size is large enough, I am confident that the compile time cost will be very high. But I am not very sure whether that is equally true for a live interval which has small number of segments spanning across a lot of basicblocks.</div><div><br></div><div>With that being said, I think changing live range size to SplitAnalysis::getNumLiveBlocks may still be a good choice because shouldRegionSplitForVirtReg will only be false if isTriviallyReMaterializable is true. Even if we give up splitting but the actual splitting cost is not high, we won't lose much because the instruction is rematerializable, but generally the change will help reducing compile time cost.</div><div><br></div><div>Thanks,</div><div>Wei.</div><div>  </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 3, 2020 at 12:05 PM Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;">Hi Wei,<div><br></div><div>I've refactored the heuristic you added in 40c4aa76 to make it more tweak-able by the backends.</div><div><br></div><div>I noticed something odd when doing the refactoring and checked back at the review (<a href="https://reviews.llvm.org/D49353" target="_blank">https://reviews.llvm.org/D49353</a>) to see how this was discussed back then and whether this was the intended behavior. Basically, I was wondering if `LiveInterval::size` was what you meant to check.</div><div><br></div><div>Like Matthias said, this returns the number of segments and AFAIU, you used that as a proxy for live-intervals that span a lot of basic block.</div><div>If that’s truly the intent, I believe the current heuristic could be improved because having a lot of segments means that the live-interval covers a lot of basic blocks but the converse is not true: covering a lot of basic blocks doesn’t necessarily means having a lot of segments.</div><div><br></div><div>If you want to check for that, instead I would recommend that you use SplitAnalysis::getNumLiveBlocks<span style="font-family:Menlo;font-size:11px;font-variant-ligatures:no-common-ligatures">. </span>Right now I did not put SA in this hook, but we could if we want to go down that road. (I might just do that actually.)</div><div><br></div><div>Anyway, just a heads-up in case you wanted to double check.</div><div><br></div><div>Cheers,</div><div>-Quentin</div><div><br></div><div><br><div><br><blockquote type="cite"><div>On Feb 3, 2020, at 11:30 AM, Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br><div><div><br>Author: Quentin Colombet<br>Date: 2020-02-03T11:30:35-08:00<br>New Revision: f26ff8c9df79a6905903be58864fb27c52374ab8<br><br>URL: <a href="https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8" target="_blank">https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8</a><br>DIFF: <a href="https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8.diff" target="_blank">https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8.diff</a><br><br>LOG: [TargetRegisterInfo] Make the heuristic to skip region split overridable by the target<br><br>RegAllocGreedy uses a fairly compile time intensive splitting heuristic<br>called region splitting. This heuristic was disabled via another heuristic<br>when it is likely that it won't be worth the compile time. The only way<br>to control this other heuristic was via a command line option (huge-size-for-split).<br><br>This commit gives more control on this heuristic by making it overridable<br>by the target using a target hook in TargetRegisterInfo called<br>shouldRegionSplitForVirtReg.<br><br>The default implementation of this hook keeps the heuristic as it was<br>before this patch.<br><br>Added: <br><br><br>Modified: <br>    llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br>    llvm/lib/CodeGen/RegAllocGreedy.cpp<br>    llvm/lib/CodeGen/TargetRegisterInfo.cpp<br><br>Removed: <br><br><br><br>################################################################################<br>diff  --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br>index ae46c2971457..658e3263559a 100644<br>--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br>+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br>@@ -40,6 +40,7 @@ class MachineInstr;<br> class RegScavenger;<br> class VirtRegMap;<br> class LiveIntervals;<br>+class LiveInterval;<br><br> class TargetRegisterClass {<br> public:<br>@@ -952,6 +953,12 @@ class TargetRegisterInfo : public MCRegisterInfo {<br>                               LiveIntervals &LIS) const<br>   { return true; }<br><br>+  /// Region split has a high compile time cost especially for large live range.<br>+  /// This method is used to decide whether or not \p VirtReg should<br>+  /// go through this expensive splitting heuristic.<br>+  virtual bool shouldRegionSplitForVirtReg(const MachineFunction &MF,<br>+                                           const LiveInterval &VirtReg) const;<br>+<br>   //===--------------------------------------------------------------------===//<br>   /// Debug information queries.<br><br><br>diff  --git a/llvm/lib/CodeGen/RegAllocGreedy.cpp b/llvm/lib/CodeGen/RegAllocGreedy.cpp<br>index 27de7fe45887..9573b91acd6d 100644<br>--- a/llvm/lib/CodeGen/RegAllocGreedy.cpp<br>+++ b/llvm/lib/CodeGen/RegAllocGreedy.cpp<br>@@ -124,12 +124,6 @@ static cl::opt<bool> EnableDeferredSpilling(<br>              "variable because of other evicted variables."),<br>     cl::init(false));<br><br>-static cl::opt<unsigned><br>-    HugeSizeForSplit("huge-size-for-split", cl::Hidden,<br>-                     cl::desc("A threshold of live range size which may cause "<br>-                              "high compile time cost in global splitting."),<br>-                     cl::init(5000));<br>-<br> // FIXME: Find a good default for this flag and remove the flag.<br> static cl::opt<unsigned><br> CSRFirstTimeCost("regalloc-csr-first-time-cost",<br>@@ -486,7 +480,6 @@ class RAGreedy : public MachineFunctionPass,<br>                     const SmallVirtRegSet&);<br>   unsigned tryRegionSplit(LiveInterval&, AllocationOrder&,<br>                           SmallVectorImpl<unsigned>&);<br>-  unsigned isSplitBenefitWorthCost(LiveInterval &VirtReg);<br>   /// Calculate cost of region splitting.<br>   unsigned calculateRegionSplitCost(LiveInterval &VirtReg,<br>                                     AllocationOrder &Order,<br>@@ -1815,20 +1808,9 @@ void RAGreedy::splitAroundRegion(LiveRangeEdit &LREdit,<br>     MF->verify(this, "After splitting live range around region");<br> }<br><br>-// Global split has high compile time cost especially for large live range.<br>-// Return false for the case here where the potential benefit will never<br>-// worth the cost.<br>-unsigned RAGreedy::isSplitBenefitWorthCost(LiveInterval &VirtReg) {<br>-  MachineInstr *MI = MRI->getUniqueVRegDef(VirtReg.reg);<br>-  if (MI && TII->isTriviallyReMaterializable(*MI, AA) &&<br>-      VirtReg.size() > HugeSizeForSplit)<br>-    return false;<br>-  return true;<br>-}<br>-<br> unsigned RAGreedy::tryRegionSplit(LiveInterval &VirtReg, AllocationOrder &Order,<br>                                   SmallVectorImpl<unsigned> &NewVRegs) {<br>-  if (!isSplitBenefitWorthCost(VirtReg))<br>+  if (!TRI->shouldRegionSplitForVirtReg(*MF, VirtReg))<br>     return 0;<br>   unsigned NumCands = 0;<br>   BlockFrequency SpillCost = calcSpillCost();<br><br>diff  --git a/llvm/lib/CodeGen/TargetRegisterInfo.cpp b/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br>index e5592c31098a..1c582ff06560 100644<br>--- a/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br>+++ b/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br>@@ -13,19 +13,22 @@<br> #include "llvm/CodeGen/TargetRegisterInfo.h"<br> #include "llvm/ADT/ArrayRef.h"<br> #include "llvm/ADT/BitVector.h"<br>-#include "llvm/ADT/SmallSet.h"<br> #include "llvm/ADT/STLExtras.h"<br>+#include "llvm/ADT/SmallSet.h"<br> #include "llvm/ADT/StringExtras.h"<br> #include "llvm/CodeGen/MachineFrameInfo.h"<br> #include "llvm/CodeGen/MachineFunction.h"<br> #include "llvm/CodeGen/MachineRegisterInfo.h"<br>+#include "llvm/CodeGen/LiveInterval.h"<br> #include "llvm/CodeGen/TargetFrameLowering.h"<br>+#include "llvm/CodeGen/TargetInstrInfo.h"<br> #include "llvm/CodeGen/TargetSubtargetInfo.h"<br> #include "llvm/CodeGen/VirtRegMap.h"<br> #include "llvm/Config/llvm-config.h"<br> #include "llvm/IR/Attributes.h"<br> #include "llvm/IR/Function.h"<br> #include "llvm/MC/MCRegisterInfo.h"<br>+#include "llvm/Support/CommandLine.h"<br> #include "llvm/Support/Compiler.h"<br> #include "llvm/Support/Debug.h"<br> #include "llvm/Support/MachineValueType.h"<br>@@ -39,6 +42,12 @@<br><br> using namespace llvm;<br><br>+static cl::opt<unsigned><br>+    HugeSizeForSplit("huge-size-for-split", cl::Hidden,<br>+                     cl::desc("A threshold of live range size which may cause "<br>+                              "high compile time cost in global splitting."),<br>+                     cl::init(5000));<br>+<br> TargetRegisterInfo::TargetRegisterInfo(const TargetRegisterInfoDesc *ID,<br>                              regclass_iterator RCB, regclass_iterator RCE,<br>                              const char *const *SRINames,<br>@@ -55,6 +64,17 @@ TargetRegisterInfo::TargetRegisterInfo(const TargetRegisterInfoDesc *ID,<br><br> TargetRegisterInfo::~TargetRegisterInfo() = default;<br><br>+bool TargetRegisterInfo::shouldRegionSplitForVirtReg(<br>+    const MachineFunction &MF, const LiveInterval &VirtReg) const {<br>+  const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();<br>+  const MachineRegisterInfo &MRI = MF.getRegInfo();<br>+  MachineInstr *MI = MRI.getUniqueVRegDef(VirtReg.reg);<br>+  if (MI && TII->isTriviallyReMaterializable(*MI) &&<br>+      VirtReg.size() > HugeSizeForSplit)<br>+    return false;<br>+  return true;<br>+}<br>+<br> void TargetRegisterInfo::markSuperRegs(BitVector &RegisterSet, unsigned Reg)<br>     const {<br>   for (MCSuperRegIterator AI(Reg, this, true); AI.isValid(); ++AI)<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" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br></div></div></blockquote></div><br></div></div></blockquote></div>