<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Wei,<div class=""><br class=""></div><div class="">I've refactored the heuristic you added in 40c4aa76 to make it more tweak-able by the backends.</div><div class=""><br class=""></div><div class="">I noticed something odd when doing the refactoring and checked back at the review (<a href="https://reviews.llvm.org/D49353" class="">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 class=""><br class=""></div><div class="">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 class="">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 class=""><br class=""></div><div class="">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;" class="">. </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 class=""><br class=""></div><div class="">Anyway, just a heads-up in case you wanted to double check.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">-Quentin</div><div class=""><br class=""></div><div class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Feb 3, 2020, at 11:30 AM, Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><br class="">Author: Quentin Colombet<br class="">Date: 2020-02-03T11:30:35-08:00<br class="">New Revision: f26ff8c9df79a6905903be58864fb27c52374ab8<br class=""><br class="">URL: <a href="https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8" class="">https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8</a><br class="">DIFF: <a href="https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8.diff" class="">https://github.com/llvm/llvm-project/commit/f26ff8c9df79a6905903be58864fb27c52374ab8.diff</a><br class=""><br class="">LOG: [TargetRegisterInfo] Make the heuristic to skip region split overridable by the target<br class=""><br class="">RegAllocGreedy uses a fairly compile time intensive splitting heuristic<br class="">called region splitting. This heuristic was disabled via another heuristic<br class="">when it is likely that it won't be worth the compile time. The only way<br class="">to control this other heuristic was via a command line option (huge-size-for-split).<br class=""><br class="">This commit gives more control on this heuristic by making it overridable<br class="">by the target using a target hook in TargetRegisterInfo called<br class="">shouldRegionSplitForVirtReg.<br class=""><br class="">The default implementation of this hook keeps the heuristic as it was<br class="">before this patch.<br class=""><br class="">Added: <br class=""><br class=""><br class="">Modified: <br class=""> llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br class=""> llvm/lib/CodeGen/RegAllocGreedy.cpp<br class=""> llvm/lib/CodeGen/TargetRegisterInfo.cpp<br class=""><br class="">Removed: <br class=""><br class=""><br class=""><br class="">################################################################################<br class="">diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br class="">index ae46c2971457..658e3263559a 100644<br class="">--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br class="">+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h<br class="">@@ -40,6 +40,7 @@ class MachineInstr;<br class=""> class RegScavenger;<br class=""> class VirtRegMap;<br class=""> class LiveIntervals;<br class="">+class LiveInterval;<br class=""><br class=""> class TargetRegisterClass {<br class=""> public:<br class="">@@ -952,6 +953,12 @@ class TargetRegisterInfo : public MCRegisterInfo {<br class=""> LiveIntervals &LIS) const<br class=""> { return true; }<br class=""><br class="">+ /// Region split has a high compile time cost especially for large live range.<br class="">+ /// This method is used to decide whether or not \p VirtReg should<br class="">+ /// go through this expensive splitting heuristic.<br class="">+ virtual bool shouldRegionSplitForVirtReg(const MachineFunction &MF,<br class="">+ const LiveInterval &VirtReg) const;<br class="">+<br class=""> //===--------------------------------------------------------------------===//<br class=""> /// Debug information queries.<br class=""><br class=""><br class="">diff --git a/llvm/lib/CodeGen/RegAllocGreedy.cpp b/llvm/lib/CodeGen/RegAllocGreedy.cpp<br class="">index 27de7fe45887..9573b91acd6d 100644<br class="">--- a/llvm/lib/CodeGen/RegAllocGreedy.cpp<br class="">+++ b/llvm/lib/CodeGen/RegAllocGreedy.cpp<br class="">@@ -124,12 +124,6 @@ static cl::opt<bool> EnableDeferredSpilling(<br class=""> "variable because of other evicted variables."),<br class=""> cl::init(false));<br class=""><br class="">-static cl::opt<unsigned><br class="">- HugeSizeForSplit("huge-size-for-split", cl::Hidden,<br class="">- cl::desc("A threshold of live range size which may cause "<br class="">- "high compile time cost in global splitting."),<br class="">- cl::init(5000));<br class="">-<br class=""> // FIXME: Find a good default for this flag and remove the flag.<br class=""> static cl::opt<unsigned><br class=""> CSRFirstTimeCost("regalloc-csr-first-time-cost",<br class="">@@ -486,7 +480,6 @@ class RAGreedy : public MachineFunctionPass,<br class=""> const SmallVirtRegSet&);<br class=""> unsigned tryRegionSplit(LiveInterval&, AllocationOrder&,<br class=""> SmallVectorImpl<unsigned>&);<br class="">- unsigned isSplitBenefitWorthCost(LiveInterval &VirtReg);<br class=""> /// Calculate cost of region splitting.<br class=""> unsigned calculateRegionSplitCost(LiveInterval &VirtReg,<br class=""> AllocationOrder &Order,<br class="">@@ -1815,20 +1808,9 @@ void RAGreedy::splitAroundRegion(LiveRangeEdit &LREdit,<br class=""> MF->verify(this, "After splitting live range around region");<br class=""> }<br class=""><br class="">-// Global split has high compile time cost especially for large live range.<br class="">-// Return false for the case here where the potential benefit will never<br class="">-// worth the cost.<br class="">-unsigned RAGreedy::isSplitBenefitWorthCost(LiveInterval &VirtReg) {<br class="">- MachineInstr *MI = MRI->getUniqueVRegDef(VirtReg.reg);<br class="">- if (MI && TII->isTriviallyReMaterializable(*MI, AA) &&<br class="">- VirtReg.size() > HugeSizeForSplit)<br class="">- return false;<br class="">- return true;<br class="">-}<br class="">-<br class=""> unsigned RAGreedy::tryRegionSplit(LiveInterval &VirtReg, AllocationOrder &Order,<br class=""> SmallVectorImpl<unsigned> &NewVRegs) {<br class="">- if (!isSplitBenefitWorthCost(VirtReg))<br class="">+ if (!TRI->shouldRegionSplitForVirtReg(*MF, VirtReg))<br class=""> return 0;<br class=""> unsigned NumCands = 0;<br class=""> BlockFrequency SpillCost = calcSpillCost();<br class=""><br class="">diff --git a/llvm/lib/CodeGen/TargetRegisterInfo.cpp b/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br class="">index e5592c31098a..1c582ff06560 100644<br class="">--- a/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br class="">+++ b/llvm/lib/CodeGen/TargetRegisterInfo.cpp<br class="">@@ -13,19 +13,22 @@<br class=""> #include "llvm/CodeGen/TargetRegisterInfo.h"<br class=""> #include "llvm/ADT/ArrayRef.h"<br class=""> #include "llvm/ADT/BitVector.h"<br class="">-#include "llvm/ADT/SmallSet.h"<br class=""> #include "llvm/ADT/STLExtras.h"<br class="">+#include "llvm/ADT/SmallSet.h"<br class=""> #include "llvm/ADT/StringExtras.h"<br class=""> #include "llvm/CodeGen/MachineFrameInfo.h"<br class=""> #include "llvm/CodeGen/MachineFunction.h"<br class=""> #include "llvm/CodeGen/MachineRegisterInfo.h"<br class="">+#include "llvm/CodeGen/LiveInterval.h"<br class=""> #include "llvm/CodeGen/TargetFrameLowering.h"<br class="">+#include "llvm/CodeGen/TargetInstrInfo.h"<br class=""> #include "llvm/CodeGen/TargetSubtargetInfo.h"<br class=""> #include "llvm/CodeGen/VirtRegMap.h"<br class=""> #include "llvm/Config/llvm-config.h"<br class=""> #include "llvm/IR/Attributes.h"<br class=""> #include "llvm/IR/Function.h"<br class=""> #include "llvm/MC/MCRegisterInfo.h"<br class="">+#include "llvm/Support/CommandLine.h"<br class=""> #include "llvm/Support/Compiler.h"<br class=""> #include "llvm/Support/Debug.h"<br class=""> #include "llvm/Support/MachineValueType.h"<br class="">@@ -39,6 +42,12 @@<br class=""><br class=""> using namespace llvm;<br class=""><br class="">+static cl::opt<unsigned><br class="">+ HugeSizeForSplit("huge-size-for-split", cl::Hidden,<br class="">+ cl::desc("A threshold of live range size which may cause "<br class="">+ "high compile time cost in global splitting."),<br class="">+ cl::init(5000));<br class="">+<br class=""> TargetRegisterInfo::TargetRegisterInfo(const TargetRegisterInfoDesc *ID,<br class=""> regclass_iterator RCB, regclass_iterator RCE,<br class=""> const char *const *SRINames,<br class="">@@ -55,6 +64,17 @@ TargetRegisterInfo::TargetRegisterInfo(const TargetRegisterInfoDesc *ID,<br class=""><br class=""> TargetRegisterInfo::~TargetRegisterInfo() = default;<br class=""><br class="">+bool TargetRegisterInfo::shouldRegionSplitForVirtReg(<br class="">+ const MachineFunction &MF, const LiveInterval &VirtReg) const {<br class="">+ const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();<br class="">+ const MachineRegisterInfo &MRI = MF.getRegInfo();<br class="">+ MachineInstr *MI = MRI.getUniqueVRegDef(VirtReg.reg);<br class="">+ if (MI && TII->isTriviallyReMaterializable(*MI) &&<br class="">+ VirtReg.size() > HugeSizeForSplit)<br class="">+ return false;<br class="">+ return true;<br class="">+}<br class="">+<br class=""> void TargetRegisterInfo::markSuperRegs(BitVector &RegisterSet, unsigned Reg)<br class=""> const {<br class=""> for (MCSuperRegIterator AI(Reg, this, true); AI.isValid(); ++AI)<br class=""><br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></div></blockquote></div><br class=""></div></body></html>