On Aug 13, 2012, at 8:34 AM, Sergei Larin <slarin at codeaurora.org> wrote:> Andy, > > Yes, this is what Arnold has suggested also, and from this point it looks > like it should work, but it will require parsing the bundle every time we > care to know whether this is a real use or a conditional def. This might > become awkward... but I guess I should provide a better use case to prove my > point.Sergei, the MachineInstr properties are exposed at the bundle level, we just don't seem to have the property that you need yet. Jakob: would it be generally useful to define either an MC instr or operand flag that indicates a conditionally written register? -Andy>> -----Original Message----- >> From: Andrew Trick [mailto:atrick at apple.com] >> Sent: Thursday, August 09, 2012 7:09 PM >> To: Sergei Larin >> Cc: LLVM Developers Mailing List; Jakob Stoklund Olesen >> Subject: Re: MI bundle liveness attributes >> >> Hi Sergei. If an instruction conditionally writes R0 then I think it >> needs to implicitly use R0 for proper liveness >> >> Andy >> >> On Aug 9, 2012, at 9:48 AM, Sergei Larin <slarin at codeaurora.org> wrote: >> >>> >>> Hello everyone, >>> >>> Let me (re)present a question that might have previously been >>> discussed, but did not result in any code (AFIK). >>> >>> How do we represent a _conditional_ assignment (def) in a bundle MI? >>> >>> More contents - currently we expose internal def/use/kill >> information >>> to a bundle header - something like this: >>> >>> >>> BUNDLE %PC<imp-def>, %R0<imp-def>, %P0<imp-use,kill>, %R16<imp-use> >>> * %R0<def> = LDriuh_cdnNotPt %P0<kill,internal>, %R16, 0; >>> * %P0<def> = CMPEQri %R16, 0; >>> >>> Here CMPEQri is a compare to a predicate register instruction, and >>> LDriuh_cdnNotPt is a _conditional_ load, which might or might not >> Take >>> place based on the outcome of the compare... As such R0 might or >> might >>> not be defined in this bundle, which obviously changes the liveness >>> update process. >>> >>> My question, do we need another attribute along with isImplicit and >>> isEarlyClobber etc. to designate a conditional def? Furthermore, >>> depending on architectural details we well might have a conditional >>> use as well... and what about the individual (unbundled) def/use? >> Should this: >>> >>> %R0<def> = LDriuh_cdnNotPt %P0<kill,internal>, %R16, 0; >>> >>> ...become this: >>> >>> %R0<def-cond> = LDriuh_cdnNotPt %P0<kill,internal>, %R16, 0; >>> >>> or even: >>> >>> %R0<def-cond> = LDriuh_cdnNotPt %P0<kill,internal>, %R16<use-cond>, >> 0; >>> >>> So, if I am missing something in current implementation or an >> ongoing >>> discussions (and that is entirely possible since I am just back after >>> vacation), please let me know how to achieve this functionality, but >>> if this is something missing in implementation, let's discuss how do >>> we want to realize it. >>> >>> Thanks. >>> >>> Sergei Larin >>> >>> -- >>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum. >>> >>> >
On Aug 15, 2012, at 10:17 AM, Andrew Trick <atrick at apple.com> wrote:> > On Aug 13, 2012, at 8:34 AM, Sergei Larin <slarin at codeaurora.org> wrote: >> Andy, >> >> Yes, this is what Arnold has suggested also, and from this point it looks >> like it should work, but it will require parsing the bundle every time we >> care to know whether this is a real use or a conditional def. This might >> become awkward... but I guess I should provide a better use case to prove my >> point. > > Sergei, the MachineInstr properties are exposed at the bundle level, we just don't seem to have the property that you need yet. > > Jakob: would it be generally useful to define either an MC instr or operand flag that indicates a conditionally written register?I don't think that is necessary. ARM already does post-RA predication and bundling. It just adds an <imp-def> operand for the predicated defs. We can't currently represent predicated instructions before register allocation because SSA form requires different input and output virtual registers, but that doesn't seem to be Sergei's problem. But I agree with Sergei that he needs to provide a better use case ;-) /jakob
Andy, Jacob,
I have ported Hexagon MI scheduler to use the new scheduler
infrastructure, but one of my tests triggers an assert in LiveInterval
update. On the surface it does not make much sense to me, so I wonder if
this is something you readily recognize, before I try to prop it open...
The assert is:
lib/CodeGen/LiveInterval.cpp:266: llvm::LiveRange*
llvm::LiveInterval::addRangeFrom(llvm::LiveRange, llvm::LiveRange*):
Assertion `B->end <= Start && "Cannot overlap two LiveRanges
with differing
ValID's" " (did you def the same reg twice in a
MachineInstr?)"' failed.
has this following call stack:
#3 in llvm::LiveInterval::addRangeFrom (this=0x46e99a0, LR=...,
From=0x46ee4b0) at lib/CodeGen/LiveInterval.cpp:266
#4 in llvm::LiveInterval::addRange (this=0x46e99a0, LR=...) at
include/llvm/CodeGen/LiveInterval.h:384
#5 in llvm::LiveIntervals::HMEditor::moveInternalFrom (this=0x7fffffff8010,
OldIdx=..., P=...) at lib/CodeGen/LiveIntervalAnalysis.cpp:1220
#6 in llvm::LiveIntervals::HMEditor::moveAllInternalFrom
(this=0x7fffffff8010, OldIdx=..., Internal=...) at
lib/CodeGen/LiveIntervalAnalysis.cpp:1226
#7 in llvm::LiveIntervals::HMEditor::moveAllRangesFrom
(this=0x7fffffff8010, MI=0x462ab80, OldIdx=...) at
lib/CodeGen/LiveIntervalAnalysis.cpp:938
#8 in llvm::LiveIntervals::handleMove (this=0x448b6f0, MI=0x462ab80) at
lib/CodeGen/LiveIntervalAnalysis.cpp:1388
#9 in llvm::VLIWMachineScheduler::moveInstruction (this=0x46b0f50,
MI=0x462ab80, InsertPos=...) at
lib/Target/Hexagon/HexagonMachineScheduler.cpp:120
#10 in llvm::VLIWMachineScheduler::schedule (this=0x46b0f50) at
lib/Target/Hexagon/HexagonMachineScheduler.cpp:378
#11 in (anonymous namespace)::MachineScheduler::runOnMachineFunction
(this=0x448c730, mf=...) at lib/CodeGen/MachineScheduler.cpp:263
#12 in llvm::MachineFunctionPass::runOnFunction (this=0x448c770, F=...) at
lib/CodeGen/MachineFunctionPass.cpp:33
For the purpose of this question, you can assume HexagonMachineScheduler.cpp
== MachineScheduler.cpp and VLIWMachineScheduler == ScheduleDAGMI
The instruction being moved is a simple call:
let isCall = 1, neverHasSideEffects = 1,
Defs = [D0, D1, D2, D3, D4, D5, D6, D7, R28, R31,
P0, P1, P2, P3, LC0, LC1, SA0, SA1, USR] in {
def CALLv3 : JInst<(outs), (ins calltarget:$dst),
"call $dst", []>, Requires<[HasV3T]>;
}
CALLv3 <ga:@printf>, %D0<imp-def,dead>, %D1<imp-def,dead>,
%D2<imp-def,dead>, %R31<imp-def>, %R0<imp-use,kill>, ...
Another clue - slot renumbering just took place:
*** Renumbered SlotIndexes 1056-2120 ***
...
*** Renumbered SlotIndexes 1068-2140 ***
...and the first move after that produces the assert.
(gdb) p Start.dump()
1080r
(gdb) p B->end.dump()
1092r
(gdb) p *LR.valno
$7 = {id = 61, def = {lie = {Value = 74160402}}}
(gdb) p *B->valno
$9 = {id = 0, def = {lie = {Value = 73472560}}}
I have not yet succeeded to reproduce it on anything other than my branch of
Hexagon, but I can try...
This is definitely some corner case, since it is a singular failure in a
very large test suite. Really hope it rings a bell for you.
Thanks.
Sergei
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
Andy,
I've described that issue (see below) when you were out of town... I think
I am getting more context on it. Please take a look...
So, in short, when the new MI scheduler performs move of an instruction, it
does something like this:
// Move the instruction to its new location in the instruction stream.
MachineInstr *MI = SU->getInstr();
if (IsTopNode) {
assert(SU->isTopReady() && "node still has unscheduled
dependencies");
if (&*CurrentTop == MI)
<<<<<<<<<<<<<<<<<< Here
we make
sure that CurrentTop != MI.
CurrentTop = nextIfDebug(++CurrentTop, CurrentBottom);
else {
moveInstruction(MI, CurrentTop);
TopRPTracker.setPos(MI);
}
...
But in moveInstruction we use
// Update the instruction stream.
BB->splice(InsertPos, BB, MI);
And splice as far as I understand moves MI to the location right __before__
InsertPos. Our previous check made sure that InsertPos != MI,
But I do hit a case, when MI == InsertPos--, and effectively tries to swap
instruction with itself... which make live update mechanism very unhappy.
If I am missing something in intended logic, please explain, otherwise it
looks like we need to adjust that check to make sure we never even
considering this situation (swapping with self).
I also wonder if we want explicit check in live update with more meaningful
assert :)
Thanks.
Sergei
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
> On Behalf Of Sergei Larin
> Sent: Friday, August 17, 2012 3:32 PM
> To: 'Andrew Trick'; 'Jakob Olesen'
> Cc: 'LLVM Developers Mailing List'
> Subject: [LLVMdev] Assert in LiveInterval update
>
>
> Andy, Jacob,
>
> I have ported Hexagon MI scheduler to use the new scheduler
> infrastructure, but one of my tests triggers an assert in LiveInterval
> update. On the surface it does not make much sense to me, so I wonder
> if this is something you readily recognize, before I try to prop it
> open...
>
> The assert is:
>
> lib/CodeGen/LiveInterval.cpp:266: llvm::LiveRange*
> llvm::LiveInterval::addRangeFrom(llvm::LiveRange, llvm::LiveRange*):
> Assertion `B->end <= Start && "Cannot overlap two
LiveRanges with
> differing ValID's" " (did you def the same reg twice in a
> MachineInstr?)"' failed.
>
> has this following call stack:
>
> #3 in llvm::LiveInterval::addRangeFrom (this=0x46e99a0, LR=...,
> From=0x46ee4b0) at lib/CodeGen/LiveInterval.cpp:266
> #4 in llvm::LiveInterval::addRange (this=0x46e99a0, LR=...) at
> include/llvm/CodeGen/LiveInterval.h:384
> #5 in llvm::LiveIntervals::HMEditor::moveInternalFrom
> (this=0x7fffffff8010, OldIdx=..., P=...) at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1220
> #6 in llvm::LiveIntervals::HMEditor::moveAllInternalFrom
> (this=0x7fffffff8010, OldIdx=..., Internal=...) at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1226
> #7 in llvm::LiveIntervals::HMEditor::moveAllRangesFrom
> (this=0x7fffffff8010, MI=0x462ab80, OldIdx=...) at
> lib/CodeGen/LiveIntervalAnalysis.cpp:938
> #8 in llvm::LiveIntervals::handleMove (this=0x448b6f0, MI=0x462ab80)
> at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1388
> #9 in llvm::VLIWMachineScheduler::moveInstruction (this=0x46b0f50,
> MI=0x462ab80, InsertPos=...) at
> lib/Target/Hexagon/HexagonMachineScheduler.cpp:120
> #10 in llvm::VLIWMachineScheduler::schedule (this=0x46b0f50) at
> lib/Target/Hexagon/HexagonMachineScheduler.cpp:378
> #11 in (anonymous namespace)::MachineScheduler::runOnMachineFunction
> (this=0x448c730, mf=...) at lib/CodeGen/MachineScheduler.cpp:263
> #12 in llvm::MachineFunctionPass::runOnFunction (this=0x448c770, F=...)
> at
> lib/CodeGen/MachineFunctionPass.cpp:33
>
>
> For the purpose of this question, you can assume
> HexagonMachineScheduler.cpp == MachineScheduler.cpp and
> VLIWMachineScheduler == ScheduleDAGMI
>
>
> The instruction being moved is a simple call:
>
> let isCall = 1, neverHasSideEffects = 1,
> Defs = [D0, D1, D2, D3, D4, D5, D6, D7, R28, R31,
> P0, P1, P2, P3, LC0, LC1, SA0, SA1, USR] in {
> def CALLv3 : JInst<(outs), (ins calltarget:$dst),
> "call $dst", []>, Requires<[HasV3T]>; }
>
> CALLv3 <ga:@printf>, %D0<imp-def,dead>,
%D1<imp-def,dead>, %D2<imp-
> def,dead>, %R31<imp-def>, %R0<imp-use,kill>, ...
>
> Another clue - slot renumbering just took place:
>
> *** Renumbered SlotIndexes 1056-2120 *** ...
>
> *** Renumbered SlotIndexes 1068-2140 ***
>
> ...and the first move after that produces the assert.
>
> (gdb) p Start.dump()
> 1080r
> (gdb) p B->end.dump()
> 1092r
> (gdb) p *LR.valno
> $7 = {id = 61, def = {lie = {Value = 74160402}}}
> (gdb) p *B->valno
> $9 = {id = 0, def = {lie = {Value = 73472560}}}
>
> I have not yet succeeded to reproduce it on anything other than my
> branch of Hexagon, but I can try...
> This is definitely some corner case, since it is a singular failure in
> a very large test suite. Really hope it rings a bell for you.
>
> Thanks.
>
> Sergei
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev