<div dir="ltr">Hi Krzystof,<div><br></div><div>I'm not sure what the problem is here. For me, the function name "<span style="font-size:13.1999998092651px;line-height:19.7999992370605px">MBB::getFirstTerminator" you quoted implies that there may be more than one terminator. So it not returning the last terminator doesn't seem surprising at all.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">In fact, at this stage a conditional branch (not to a fallthrough block) is represented as a sequence of "B.COND; B", where both are terminators and one is predicated. I don't see how conditional returns are any different.</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">Cheers,</span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px"><br></span></div><div><span style="font-size:13.1999998092651px;line-height:19.7999992370605px">James</span></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, 12 Aug 2015 at 15:43 Krzysztof Parzyszek via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Doh. I missed the list in my first reply...  Here's the replay of the<br>
conversation:<br>
<br>
<br>
<br>
----- Renato:<br>
<br>
On 10 August 2015 at 14:05, Krzysztof Parzyszek via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
 > -->     %SP<def,tied1> = t2LDMIA_RET %SP<tied0>, pred:8, pred:%CPSR,<br>
 > %R7<def>, %PC<def>, %SP<imp-use,undef>, %R7<imp-use,undef>,<br>
 > %PC<imp-use,undef><br>
 ><br>
 > Here the instruction t2LDMIA_RET is a terminator and yet it's<br>
followed by a<br>
 > non-terminator tBLXi.  This looks wrong.  Does anyone have any<br>
comments on<br>
 > this?<br>
<br>
Isn't it because one of the predicates is CPSR, which means it's a<br>
conditional instruction, so not really a terminator?<br>
<br>
This lowers to the expected:<br>
<br>
     str lr, [sp, #-4]!<br>
     cmp r1, #0<br>
     it ne<br>
     cmpne r0, #3<br>
     bhi .LBB0_2 <-- Turned into a conditional jump<br>
<br>
     bl bar<br>
<br>
.LBB0_2:<br>
     ldr lr, [sp], #4<br>
     bx lr<br>
<br>
cheers,<br>
--renato<br>
<br>
<br>
<br>
----- Me:<br>
<br>
On 8/11/2015 8:32 AM, Renato Golin wrote:<br>
 > On 10 August 2015 at 14:05, Krzysztof Parzyszek via llvm-dev<br>
 > <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
 >> -->     %SP<def,tied1> = t2LDMIA_RET %SP<tied0>, pred:8, pred:%CPSR,<br>
 >> %R7<def>, %PC<def>, %SP<imp-use,undef>, %R7<imp-use,undef>,<br>
 >> %PC<imp-use,undef><br>
 >><br>
 >> Here the instruction t2LDMIA_RET is a terminator and yet it's<br>
followed by a<br>
 >> non-terminator tBLXi.  This looks wrong.  Does anyone have any<br>
comments on<br>
 >> this?<br>
 ><br>
 > Isn't it because one of the predicates is CPSR, which means it's a<br>
 > conditional instruction, so not really a terminator?<br>
<br>
It is marked as a terminator in the table-gen output (ARMGenInstrInfo.inc):<br>
<br>
   { 2399,       5,      1,      4,      355,<br>
0|(1ULL<<MCID::Pseudo)|(1ULL<<MCID::Return)|(1ULL<<MCID::Barrier)|(1ULL<<MCID::MayLoad)|(1ULL<<MCID::Predicable)|(1ULL<<MCID::Terminator)|(1ULL<<MCID::Variadic)|(1ULL<<MCID::UnmodeledSideEffects)|(1ULL<<MCID::ExtraDefRegAllocReq),<br>
0x0ULL, nullptr, nullptr, OperandInfo52, -1 ,nullptr },  // Inst #2399 =<br>
t2LDMIA_RET<br>
<br>
<br>
The test isTerminator only checks the MCInstrDesc:<br>
<br>
   /// Various passes use this to insert code into the bottom of a basic<br>
block,<br>
   /// but before control flow occurs.<br>
   bool isTerminator() const { return Flags & (1 << MCID::Terminator); }<br>
<br>
<br>
-Krzysztof<br>
<br>
<br>
<br>
----- Renato:<br>
<br>
On 11 August 2015 at 14:54, Krzysztof Parzyszek<br>
<<a href="mailto:kparzysz@codeaurora.org" target="_blank">kparzysz@codeaurora.org</a>> wrote:<br>
 > The test isTerminator only checks the MCInstrDesc:<br>
 ><br>
 >   /// Various passes use this to insert code into the bottom of a basic<br>
 > block,<br>
 >   /// but before control flow occurs.<br>
 >   bool isTerminator() const { return Flags & (1 << MCID::Terminator); }<br>
<br>
I'm guessing this only means "can be terminator". With conditional<br>
terminators there really isn't a way to be certain, and with so many<br>
instructions being conditional in Thumb, it's probably not a good idea<br>
to try and model it perfectly.<br>
<br>
Are you relying on isTerminator() for something and getting it wrong?<br>
<br>
--renato<br>
<br>
<br>
<br>
----- Me:<br>
<br>
On 8/11/2015 9:10 AM, Renato Golin wrote:<br>
 > On 11 August 2015 at 14:54, Krzysztof Parzyszek<br>
<<a href="mailto:kparzysz@codeaurora.org" target="_blank">kparzysz@codeaurora.org</a>> wrote:<br>
 >> The test isTerminator only checks the MCInstrDesc:<br>
 >><br>
 >>    /// Various passes use this to insert code into the bottom of a basic<br>
 >> block,<br>
 >>    /// but before control flow occurs.<br>
 >>    bool isTerminator() const { return Flags & (1 << MCID::Terminator); }<br>
 ><br>
 > I'm guessing this only means "can be terminator". With conditional<br>
 > terminators there really isn't a way to be certain, and with so many<br>
 > instructions being conditional in Thumb, it's probably not a good idea<br>
 > to try and model it perfectly.<br>
 ><br>
 > Are you relying on isTerminator() for something and getting it wrong?<br>
<br>
This came up in our local build that has some extra code in<br>
if-conversion to predicate a "diamond", but with both sides returning.<br>
There was an unrelated problem there, and while I was working on it I<br>
found out that we can still generate a basic block with a conditional<br>
return in the middle of it.  This is the late if-conversion, so it may<br>
be that some rules are relaxed at this point.<br>
Still, MBB::getFirstTerminator would return the conditional return even<br>
though it may be followed by other instruction, and if it was to happen<br>
earlier in the optimization sequence, a lot of things could go wrong. It<br>
seems like this is not causing more trouble simply because there is no<br>
earlier optimization that could "exploit" this.<br>
<br>
To answer your question---no, at this point nothing is catastrophically<br>
wrong, but it looks like a problem waiting to happen.<br>
<br>
-Krzysztof<br>
<br>
<br>
<br>
----- Renato:<br>
<br>
On 11 August 2015 at 15:28, Krzysztof Parzyszek<br>
<<a href="mailto:kparzysz@codeaurora.org" target="_blank">kparzysz@codeaurora.org</a>> wrote:<br>
 > It seems like this is not causing more trouble simply because there is no<br>
 > earlier optimization that could "exploit" this.<br>
<br>
I see. This is worrying, but without actual hard data, it's hard to<br>
know how to fix it.<br>
<br>
<br>
 > To answer your question---no, at this point nothing is catastrophically<br>
 > wrong, but it looks like a problem waiting to happen.<br>
<br>
I'd add a comment to isTerminator() to make sure that people using it<br>
know that it can back-fire if used with predicated code.<br>
<br>
--renato<br>
<br>
<br>
<br>
----- Me:<br>
<br>
On 8/11/2015 9:36 AM, Renato Golin wrote:<br>
 ><br>
 > I'd add a comment to isTerminator() to make sure that people using it<br>
 > know that it can back-fire if used with predicated code.<br>
<br>
Hmm.  I'm not sure if this is a proper description.  A conditional<br>
branch is "predicated".  If we allow a conditional return to be in the<br>
middle of a basic block at a certain point, should we also allow (at<br>
least formally) conditional branches as well?  That would imply that<br>
there is a point in the optimization sequence after which having<br>
"exiting" instructions is allowed in the middle of the block.  Since<br>
having a conditional branch in the middle is not allowed during early<br>
optimizations, a predicated return should also be disallowed there.<br>
Can such a point be formally defined (as in "after pass XYX, is it<br>
allowed to have predicated terminators in the middle of a block")?<br>
<br>
-Krzysztof<br>
<br>
<br>
<br>
<br>
--<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,<br>
hosted by The Linux Foundation<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>         <a href="http://llvm.cs.uiuc.edu" rel="noreferrer" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>