[PATCH] D20907: [IfConversion] Bugfix: don't add Undef flag on use if reg is live.

Fri Jul 1 16:09:06 PDT 2016

Hi Jonas,

> On Jul 1, 2016, at 1:12 AM, Jonas Paulsson <paulsson at linux.vnet.ibm.com> wrote:
> 
> jonpa added a comment.
> 
>> Returned, not necessary, but clobbered, yes.
> 
> 
> OK, if the second tBL just clobbers it, I guess at that point %R0 is actually dead.
> 
>> To me the bug seems to come from the fact we thought it is live whereas it is not. Could you check why we think it is live before tMOVr?
> 
> 
> IfConverter::IfConvertTriangle() gets to work on:
> 
>  BB#11: derived from LLVM BB %if.end5
>      Live Ins: %R4 %R5 %R8 %R10 %R11
>      Predecessors according to CFG: BB#9
>  	%R9<def> = t2ADDri %R4, 32, pred:14, pred:%noreg, opt:%noreg
>  	%R0<def> = t2ADDri %R4, 16, pred:14, pred:%noreg, opt:%noreg
>  	tBL pred:14, pred:%noreg, <ga:@_ZN1B5m_fn1Ev>, <regmask %LR %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R4 %R5 %R6 %R7 %R8 %R9 %R10 %R11 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q4_Q5_Q6_Q7 %R4_R5 %R6_R7 %R8_R9 %R10_R11 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D8_D10_D12_D14 %D9_D11_D13_D15 %D9_D10 %D11_D12 %D13_D14 %D9_D10_D11_D12 %D11_D12_D13_D14>, %LR<imp-def,dead>, %SP<imp-use>, %R0<imp-use>, %SP<imp-def>
>  	%R0<def> = tMOVr %R9, pred:14, pred:%noreg
>  	tBL pred:14, pred:%noreg, <ga:@_ZN1CC1Ev>, <regmask %LR %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R0 %R4 %R5 %R6 %R7 %R8 %R9 %R10 %R11 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q4_Q5_Q6_Q7 %R4_R5 %R6_R7 %R8_R9 %R10_R11 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D8_D10_D12_D14 %D9_D11_D13_D15 %D9_D10 %D11_D12 %D13_D14 %D9_D10_D11_D12 %D11_D12_D13_D14>, %LR<imp-def,dead>, %SP<imp-use>, %R0<imp-use>, %SP<imp-def>
>  	tBL pred:14, pred:%noreg, <ga:@_ZN1CanE1A>, <regmask %LR %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R4 %R5 %R6 %R7 %R8 %R9 %R10 %R11 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q4_Q5_Q6_Q7 %R4_R5 %R6_R7 %R8_R9 %R10_R11 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D8_D10_D12_D14 %D9_D11_D13_D15 %D9_D10 %D11_D12 %D13_D14 %D9_D10_D11_D12 %D11_D12_D13_D14>, %LR<imp-def,dead>, %SP<imp-use>, %R0<imp-use>, %SP<imp-def>
>  	t2CMPri %R8<kill>, 0, pred:14, pred:%noreg, %CPSR<imp-def>
>  	t2Bcc <BB#13>, pred:0, pred:%CPSR<kill>
>      Successors according to CFG: BB#13(0x30000000 / 0x80000000 = 37.50%) BB#12(0x50000000 / 0x80000000 = 62.50%)
> 
>  BB#12: derived from LLVM BB %if.then9
>      Live Ins: %R4 %R5 %R10 %R11
>      Predecessors according to CFG: BB#11
>  	%R0<def> = tMOVr %R11<kill>, pred:1, pred:%CPSR
>  	tBL pred:14, pred:%noreg, <ga:@_ZN1F5m_fn3Ev>, <regmask %LR %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R4 %R5 %R6 %R7 %R8 %R9 %R10 %R11 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q4_Q5_Q6_Q7 %R4_R5 %R6_R7 %R8_R9 %R10_R11 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D8_D10_D12_D14 %D9_D11_D13_D15 %D9_D10 %D11_D12 %D13_D14 %D9_D10_D11_D12 %D11_D12_D13_D14>, %LR<imp-def,dead>, %SP<imp-use>, %R0<imp-use>, %SP<imp-def>
>      Successors according to CFG: BB#13(?%)
> 
>  BB#13: derived from LLVM BB %cleanup
>      Live Ins: %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R0 %R4 %R5 %R10 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D6_D8 %D7_D9 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %D14_D16 %D15_D17 %Q3_Q4 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q7_Q8 %Q1_Q2_Q3_Q4 %Q2_Q3_Q4_Q5 %Q3_Q4_Q5_Q6 %Q4_Q5_Q6_Q7 %Q5_Q6_Q7_Q8 %Q6_Q7_Q8_Q9 %Q7_Q8_Q9_Q10 %R0_R1 %R4_R5 %R10_R11 %D6_D7_D8 %D7_D8_D9 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D14_D15_D16 %D15_D16_D17 %D4_D6_D8 %D5_D7_D9 %D6_D8_D10 %D7_D9_D11 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D12_D14_D16 %D13_D15_D17 %D14_D16_D18 %D15_D17_D19 %D2_D4_D6_D8 %D3_D5_D7_D9 %D4_D6_D8_D10 %D5_D7_D9_D11 %D6_D8_D10_D12 %D7_D9_D11_D13 %D8_D10_D12_D14 %D9_D11_D13_D15 %D10_D12_D14_D16 %D11_D13_D15_D17 %D12_D14_D16_D18 %D13_D15_D17_D19 %D14_D16_D18_D20 %D15_D17_D19_D21 %D7_D8 %D9_D10 %D11_D12 %D13_D14 %D15_D16 %D5_D6_D7_D8 %D7_D8_D9_D10 %D9_D10_D11_D12 %D11_D12_D13_D14 %D13_D14_D15_D16 %D15_D16_D17_D18 %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %Q4 %Q5 %Q6 %Q7 %R4 %R5 %R10 %S16 %S17 %S18 %S19 %S20 %S21 %S22 %S23 %S24 %S25 %S26 %S27 %S28 %S29 %S30 %S31 %D6_D8 %D7_D9 %D8_D10 %D9_D11 %D10_D12 %D11_D13 %D12_D14 %D13_D15 %D14_D16 %D15_D17 %Q3_Q4 %Q4_Q5 %Q5_Q6 %Q6_Q7 %Q7_Q8 %Q1_Q2_Q3_Q4 %Q2_Q3_Q4_Q5 %Q3_Q4_Q5_Q6 %Q4_Q5_Q6_Q7 %Q5_Q6_Q7_Q8 %Q6_Q7_Q8_Q9 %Q7_Q8_Q9_Q10 %R4_R5 %R10_R11 %D6_D7_D8 %D7_D8_D9 %D8_D9_D10 %D9_D10_D11 %D10_D11_D12 %D11_D12_D13 %D12_D13_D14 %D13_D14_D15 %D14_D15_D16 %D15_D16_D17 %D4_D6_D8 %D5_D7_D9 %D6_D8_D10 %D7_D9_D11 %D8_D10_D12 %D9_D11_D13 %D10_D12_D14 %D11_D13_D15 %D12_D14_D16 %D13_D15_D17 %D14_D16_D18 %D15_D17_D19 %D2_D4_D6_D8 %D3_D5_D7_D9 %D4_D6_D8_D10 %D5_D7_D9_D11 %D6_D8_D10_D12 %D7_D9_D11_D13 %D8_D10_D12_D14 %D9_D11_D13_D15 %D10_D12_D14_D16 %D11_D13_D15_D17 %D12_D14_D16_D18 %D13_D15_D17_D19 %D14_D16_D18_D20 %D15_D17_D19_D21 %D7_D8 %D9_D10 %D11_D12 %D13_D14 %D15_D16 %D5_D6_D7_D8 %D7_D8_D9_D10 %D9_D10_D11_D12 %D11_D12_D13_D14 %D13_D14_D15_D16 %D15_D16_D17_D18
>      Predecessors according to CFG: BB#12 BB#11
>  	%R0<def> = t2MOVi 1, pred:14, pred:%noreg, opt:%noreg
>  	%R9<def> = t2ADDri %R6, 28, pred:14, pred:%noreg, opt:%noreg
>  Successors according to CFG: BB#14(?%)
> 
> Before predicating BB#12, it does
> 
>  // Initialize liveins to the first BB. These are potentially redefined by
>  // predicated instructions.
>  Redefs.init(TRI);
>  Redefs.addLiveIns(*CvtBBI->BB);
>  Redefs.addLiveIns(*NextBBI->BB);
> 
> Here %R0 gets inserted into Redefs, since it is live into BB#13!

Wait what :).
That seems wrong to me as well.

> 
> First of all, if you ask me, I'd say that the Live Ins list is suspiciously long for BB#13. I have seen and reported bad live-in lists before: https://llvm.org/bugs/show_bug.cgi?id=25263. Another point is that %R0 is actually not really live-in to BB#13, since it is overwritten unconditionally by the t2MOVi.

Agreed. Looks like the path to fix the verifier complaining is to get why this gets wrong.

> 
> Regardless of live-in lists correctness, I am thinking that IfConversion should learn to not think that a live-out reg is live into both successors here. After all, it should not be impossible to have e.g. unoptimized cases where you define a reg, and conditionally redefine it in another block, or?

Indeed, but that is conservatively correct.

> 
> BTW Quentin: I do not see your post here on Phabricator for some reason. Did you perhaps just reply to the e-mail (which perhaps should have worked…)?

I just replied to the email.
Don’t know why it does not show up on phab. I thought it was working (at least I saw it working sometimes :)).

Cheers,
-Quentin

> 
> 
> http://reviews.llvm.org/D20907
> 
> 
> 