[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:42:05 PDT 2017
> On Jun 2, 2017, at 4:22 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
>>
>> On Jun 2, 2017, at 4:08 PM, Matthias Braun <mbraun at apple.com <mailto: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.
Yep, were not required to reproduce the problem.
Removed as r304615.
Thanks
>
>>
>> - Matthias
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://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/20170602/5dfbae2d/attachment.html>
More information about the llvm-commits
mailing list