[llvm] r304603 - [RABasic] Properly update the LiveRegMatrix when LR splitting occur
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 2 16:08:59 PDT 2017
> On Jun 2, 2017, at 4:05 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
>>
>> On Jun 2, 2017, at 4:00 PM, Matthias Braun <mbraun at apple.com <mailto:mbraun at apple.com>> wrote:
>>
>>>
>>> On Jun 2, 2017, at 3:46 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>
>>> Author: qcolombet
>>> Date: Fri Jun 2 17:46:31 2017
>>> New Revision: 304603
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=304603&view=rev <http://llvm.org/viewvc/llvm-project?rev=304603&view=rev>
>>> Log:
>>> [RABasic] Properly update the LiveRegMatrix when LR splitting occur
>>>
>>> Prior to this patch we used to not touch the LiveRegMatrix while doing
>>> live-range splitting. In other words, when live-range splitting was
>>> occurring, the LiveRegMatrix was not reflecting the changes.
>>> This is generally fine because it means the query to the LiveRegMatrix
>>> will be conservately correct. However, when decisions are taken based on
>>> what is going to happen on the interferences (e.g., when we spill a
>>> register and know that it is going to be available for another one), we
>>> might hit an assertion that the color used for the assignment is still
>>> in use.
>>>
>>> This patch makes sure the changes on the live-ranges are properly
>>> reflected in the LiveRegMatrix, so the assertions don't break.
>>> An alternative could have been to remove the assertion, but it would
>>> make the invariants of the code and the general reasoning more
>>> complicated in my opnion.
>>>
>>> http://llvm.org/PR33057 <http://llvm.org/PR33057>
>>>
>>> Added:
>>> llvm/trunk/test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir
>>> Modified:
>>> llvm/trunk/lib/CodeGen/RegAllocBasic.cpp
>>>
>>> Modified: llvm/trunk/lib/CodeGen/RegAllocBasic.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocBasic.cpp?rev=304603&r1=304602&r2=304603&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/RegAllocBasic.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/RegAllocBasic.cpp Fri Jun 2 17:46:31 2017
>>> @@ -58,8 +58,9 @@ namespace {
>>> /// whenever a register is unavailable. This is not practical in production but
>>> /// provides a useful baseline both for measuring other allocators and comparing
>>> /// the speed of the basic algorithm against other styles of allocators.
>>> -class RABasic : public MachineFunctionPass, public RegAllocBase
>>> -{
>>> +class RABasic : public MachineFunctionPass,
>>> + public RegAllocBase,
>>> + private LiveRangeEdit::Delegate {
>>> // context
>>> MachineFunction *MF;
>>>
>>> @@ -72,6 +73,9 @@ class RABasic : public MachineFunctionPa
>>> // selectOrSplit().
>>> BitVector UsableRegs;
>>>
>>> + bool LRE_CanEraseVirtReg(unsigned) override;
>>> + void LRE_WillShrinkVirtReg(unsigned) override;
>>> +
>>> public:
>>> RABasic();
>>>
>>> @@ -138,6 +142,28 @@ INITIALIZE_PASS_DEPENDENCY(LiveRegMatrix
>>> INITIALIZE_PASS_END(RABasic, "regallocbasic", "Basic Register Allocator", false,
>>> false)
>>>
>>> +bool RABasic::LRE_CanEraseVirtReg(unsigned VirtReg) {
>>> + if (VRM->hasPhys(VirtReg)) {
>>> + LiveInterval &LI = LIS->getInterval(VirtReg);
>>> + Matrix->unassign(LI);
>>> + aboutToRemoveInterval(LI);
>>> + return true;
>>> + }
>>> + // Unassigned virtreg is probably in the priority queue.
>>> + // RegAllocBase will erase it after dequeueing.
>>> + return false;
>>> +}
>>> +
>>> +void RABasic::LRE_WillShrinkVirtReg(unsigned VirtReg) {
>>> + if (!VRM->hasPhys(VirtReg))
>>> + return;
>>> +
>>> + // Register is assigned, put it back on the queue for reassignment.
>>> + LiveInterval &LI = LIS->getInterval(VirtReg);
>>> + Matrix->unassign(LI);
>>> + enqueue(&LI);
>>> +}
>>> +
>>> RABasic::RABasic(): MachineFunctionPass(ID) {
>>> }
>>>
>>> @@ -207,7 +233,7 @@ bool RABasic::spillInterferences(LiveInt
>>> Matrix->unassign(Spill);
>>>
>>> // Spill the extracted interval.
>>> - LiveRangeEdit LRE(&Spill, SplitVRegs, *MF, *LIS, VRM, nullptr, &DeadRemats);
>>> + LiveRangeEdit LRE(&Spill, SplitVRegs, *MF, *LIS, VRM, this, &DeadRemats);
>>> spiller().spill(LRE);
>>> }
>>> return true;
>>> @@ -266,7 +292,7 @@ unsigned RABasic::selectOrSplit(LiveInte
>>> DEBUG(dbgs() << "spilling: " << VirtReg << '\n');
>>> if (!VirtReg.isSpillable())
>>> return ~0u;
>>> - LiveRangeEdit LRE(&VirtReg, SplitVRegs, *MF, *LIS, VRM, nullptr, &DeadRemats);
>>> + LiveRangeEdit LRE(&VirtReg, SplitVRegs, *MF, *LIS, VRM, this, &DeadRemats);
>>> spiller().spill(LRE);
>>>
>>> // The live virtual register requesting allocation was spilled, so tell
>>>
>>> Added: llvm/trunk/test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir?rev=304603&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir (added)
>>> +++ llvm/trunk/test/CodeGen/SystemZ/RAbasic-invalid-LR-update.mir Fri Jun 2 17:46:31 2017
>>> @@ -0,0 +1,279 @@
>>> +# RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -run-pass=regallocbasic %s -o - | FileCheck %s
>>> +# This test used to assert in RABasic. The problem was when we split live-ranges,
>>> +# we were not updating the LiveRegMatrix properly and the interference calculation
>>> +# wouldn't match what the assignment thought it could do.
>>> +# In other words, this test case needs to trigger live-range splitting to exercise
>>> +# the problem.
>>> +#
>>> +# PR33057
>>> +--- |
>>> + target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
>>> + target triple = "s390x--linux-gnu"
>>> +
>>> + define void @autogen_SD21418() #0 {
>>> + ret void
>>> + }
>>> +
>>> + attributes #0 = { "target-cpu"="z13" }
>>> +
>>> +...
>>> +
>>> +# CHECK: name: autogen_SD21418
>>> +# Check that at least one live-range has been split
>>> +# CHECK: id: 114, class
>>> +---
>>> +name: autogen_SD21418
>>> +alignment: 2
>>> +tracksRegLiveness: true
>>> +registers:
>>> + - { id: 0, class: vr128bit }
>>> + - { id: 1, class: vr128bit }
>>> + - { id: 2, class: vr128bit }
>>> + - { id: 3, class: vr64bit }
>>> + - { id: 4, class: gr64bit }
>>> + - { id: 5, class: vr128bit }
>>> + - { id: 6, class: grx32bit }
>>> + - { id: 7, class: vr128bit }
>>> + - { id: 8, class: vr128bit }
>>> + - { id: 9, class: gr32bit }
>>> + - { id: 10, class: gr64bit }
>>> + - { id: 11, class: vr128bit }
>>> + - { id: 12, class: fp64bit }
>>> + - { id: 13, class: vr64bit }
>>> + - { id: 14, class: vr64bit }
>>> + - { id: 15, class: gr64bit }
>>> + - { id: 16, class: gr128bit }
>>> + - { id: 17, class: gr64bit }
>>> + - { id: 18, class: gr32bit }
>>> + - { id: 19, class: gr32bit }
>>> + - { id: 20, class: gr128bit }
>>> + - { id: 21, class: gr32bit }
>>> + - { id: 22, class: gr64bit }
>>> + - { id: 23, class: gr32bit }
>>> + - { id: 24, class: gr32bit }
>>> + - { id: 25, class: gr128bit }
>>> + - { id: 26, class: grx32bit }
>>> + - { id: 27, class: gr64bit }
>>> + - { id: 28, class: gr64bit }
>>> + - { id: 29, class: vr128bit }
>>> + - { id: 30, class: vr128bit }
>>> + - { id: 31, class: gr64bit }
>>> + - { id: 32, class: gr32bit }
>>> + - { id: 33, class: gr32bit }
>>> + - { id: 34, class: gr128bit }
>>> + - { id: 35, class: gr32bit }
>>> + - { id: 36, class: vr128bit }
>>> + - { id: 37, class: gr64bit }
>>> + - { id: 38, class: gr32bit }
>>> + - { id: 39, class: gr32bit }
>>> + - { id: 40, class: gr128bit }
>>> + - { id: 41, class: gr32bit }
>>> + - { id: 42, class: addr64bit }
>>> + - { id: 43, class: grx32bit }
>>> + - { id: 44, class: addr64bit }
>>> + - { id: 45, class: vr64bit }
>>> + - { id: 46, class: vr64bit }
>>> + - { id: 47, class: gr32bit }
>>> + - { id: 48, class: gr32bit }
>>> + - { id: 49, class: grx32bit }
>>> + - { id: 50, class: vr64bit }
>>> + - { id: 51, class: gr64bit }
>>> + - { id: 52, class: grx32bit }
>>> + - { id: 53, class: gr32bit }
>>> + - { id: 54, class: gr64bit }
>>> + - { id: 55, class: grx32bit }
>>> + - { id: 56, class: gr32bit }
>>> + - { id: 57, class: gr128bit }
>>> + - { id: 58, class: gr128bit }
>>> + - { id: 59, class: gr32bit }
>>> + - { id: 60, class: gr64bit }
>>> + - { id: 61, class: grx32bit }
>>> + - { id: 62, class: gr32bit }
>>> + - { id: 63, class: gr64bit }
>>> + - { id: 64, class: grx32bit }
>>> + - { id: 65, class: gr32bit }
>>> + - { id: 66, class: gr128bit }
>>> + - { id: 67, class: gr128bit }
>>> + - { id: 68, class: grx32bit }
>>> + - { id: 69, class: gr64bit }
>>> + - { id: 70, class: gr64bit }
>>> + - { id: 71, class: vr128bit }
>>> + - { id: 72, class: vr128bit }
>>> + - { id: 73, class: gr64bit }
>>> + - { id: 74, class: grx32bit }
>>> + - { id: 75, class: gr32bit }
>>> + - { id: 76, class: gr64bit }
>>> + - { id: 77, class: grx32bit }
>>> + - { id: 78, class: gr32bit }
>>> + - { id: 79, class: gr128bit }
>>> + - { id: 80, class: gr128bit }
>>> + - { id: 81, class: gr32bit }
>>> + - { id: 82, class: vr128bit }
>>> + - { id: 83, class: gr64bit }
>>> + - { id: 84, class: grx32bit }
>>> + - { id: 85, class: gr32bit }
>>> + - { id: 86, class: gr64bit }
>>> + - { id: 87, class: grx32bit }
>>> + - { id: 88, class: gr32bit }
>>> + - { id: 89, class: gr128bit }
>>> + - { id: 90, class: gr128bit }
>>> + - { id: 91, class: gr32bit }
>>> + - { id: 92, class: grx32bit }
>>> + - { id: 93, class: gr64bit }
>>> + - { id: 94, class: gr32bit }
>>> + - { id: 95, class: gr32bit }
>>> + - { id: 96, class: gr32bit }
>>> + - { id: 97, class: gr64bit }
>>> + - { id: 98, class: gr64bit }
>>> + - { id: 99, class: grx32bit }
>>> + - { id: 100, class: grx32bit }
>>> + - { id: 101, class: gr128bit }
>>> + - { id: 102, class: gr128bit }
>>> + - { id: 103, class: gr128bit }
>>> + - { id: 104, class: gr64bit }
>>> + - { id: 105, class: gr128bit }
>>> + - { id: 106, class: gr128bit }
>>> + - { id: 107, class: gr64bit }
>>> + - { id: 108, class: gr128bit }
>>> + - { id: 109, class: gr128bit }
>>> + - { id: 110, class: gr64bit }
>>> + - { id: 111, class: gr128bit }
>>> + - { id: 112, class: gr128bit }
>>> + - { id: 113, class: gr64bit }
>>> +constants:
>>> + - id: 0
>>> + value: double 0xD55960F86F577076
>>> + alignment: 8
>>> +body: |
>>> + bb.0:
>>> + %11 = VGBM 0
>>> + %43 = LHIMux 0
>>> + %44 = LARL %const.0
>>> + %45 = VL64 %44, 0, _ :: (load 8 from constant-pool)
>>> +
>>> + bb.1:
>>> + ADJCALLSTACKDOWN 0, 0
>>> + %12 = LZDR
>>> + %f0d = COPY %12
>>> + CallBRASL $fmod, killed %f0d, undef %f2d, csr_systemz, implicit-def dead %r14d, implicit-def dead %cc, implicit-def %f0d
>>> + ADJCALLSTACKUP 0, 0
>>> + KILL killed %f0d
>>> +
>>> + bb.2:
>>> + successors: %bb.2(0x7c000000), %bb.3(0x04000000)
>> You probably could have used `-simplify-mir` to skip all those successors lines.
>
> I did.
> Look around, you’ll see that some of the successors haven’t been set. A bug in the simplifier?
Oh indeed. The fact that some are left is not a bug, the simplifier is just conservative. In this case it would change the branch probabilities. It's highly likely that this won't affect this particular test but the simplifier cannot know.
- Matthias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170602/a28ee068/attachment.html>
More information about the llvm-commits
mailing list