[llvm] r304603 - [RABasic] Properly update the LiveRegMatrix when LR splitting occur
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 2 16:22:01 PDT 2017
> On Jun 2, 2017, at 4:08 PM, Matthias Braun <mbraun at apple.com> wrote:
>
>>
>> On Jun 2, 2017, at 4:05 PM, Quentin Colombet <qcolombet at apple.com <mailto: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 <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 <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.
Good to know.
Let me try without them.
>
> - Matthias
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170602/01bf0059/attachment.html>
More information about the llvm-commits
mailing list