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