[PATCH] ARM: Fix convergence failure in constant-island pass

Bob Wilson bob.wilson at apple.com
Thu Oct 16 16:43:54 PDT 2014


This looks good. Thanks for the thorough description.

The fix itself is pretty obviously correct. I had some concerns that there might be other issues exposed in a case like this where there are very few instructions remaining to split. I generally expect the split-the-basic-block situation to come up when there is a really large block, but in this case the block is small. The reason the constant cannot be placed in this test is that we have a load with a very small offset field and the block is followed by a large constant pool. Our rules for convergence require that constants be placed at the end of an existing pool, which in this case is out of range. Akira and I discussed this in person, and we couldn’t find any other problems. Basically you can always end up with the fall-back of splitting immediately after the load instruction.

> On Oct 16, 2014, at 4:19 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:
> 
> The attached patch fixes a bug in ARM's constant island pass. The bug is in ARMConstantIslandPass.cpp:1307-1314 where the upper bound of the new water split point is computed:
> 
> // This could point off the end of the block if we've already got constant
> // pool entries following this block; only the last one is in the water list.
> // Back past any possible branches (allow for a conditional and a maximally
> // long unconditional).
> if (BaseInsertOffset + 8 >= UserBBI.postOffset()) {
>     BaseInsertOffset = UserBBI.postOffset() - UPad - 8;
>     DEBUG(dbgs() << format("Move inside block: %#x\n", BaseInsertOffset));
> }
> 
> The split point is supposed to be somewhere between the machine instruction that loads from the constant pool entry and the end of the basic block, before branch instructions. The code above is fine if the basic block is large enough and there are a sufficient number of instructions following the machine instruction. However, if the machine instruction is near the end of the basic block, BaseInsertOffset can point to the machine instruction or another instruction that precedes it, and this can lead to convergence failure (see the example below).
> 
> The attached patch fixes this bug by ensuring BaseInsertOffset is larger than the offset of the instruction following the constant-loading instruction.
> 
> 
> -    BaseInsertOffset = UserBBI.postOffset() - UPad - 8;
> +    BaseInsertOffset =
> +        std::max(UserBBI.postOffset() - UPad - 8,
> +                 UserOffset + TII->GetInstSizeInBytes(UserMI) + 1);
> 
> 
> rdar://problem/18581150
> 
> 
> This is how the bug can lead to convergence failure:
> 
> 1. Instruction VLDRS is too far from the constant pool entry cp#136 in BB#189. A new water has to be created in BB#324 after VLDRS.
> 
> BB#189: Align 2 (4 bytes)
> 	CONSTPOOL_ENTRY 345, <cp#136>, 4
> 
> ...
> 
> BB#323: Align 2 (4 bytes)
> 	CONSTPOOL_ENTRY 479, <cp#156>, 4
> 
> BB#324: derived from LLVM BB %bb35
>     Predecessors according to CFG: BB#187
> 	%D0<def> = VORRd %D9, %D9, pred:14, pred:%noreg, %S18<imp-use>
> 	%S6<def> = VLDRS <cp#345>, 0, pred:14, pred:%noreg, %D3<imp-def>; mem:LD4[ConstantPool]
> 	%D5<def> = VLDRD <cp#346>, 0, pred:14, pred:%noreg; mem:LD8[ConstantPool]
> 	t2STRi12 %R1<kill>, %R10, 0, pred:14, pred:%noreg; mem:ST4[%arg14]
> 	t2B <BB#342>, pred:14, pred:%noreg
>     Successors according to CFG: BB#342
> 
> 
> 2. However, new water (BB#325) is created before VLDRS and cp#136 is moved there, because VLDRS is too close to the end of its parent basic block. This causes all the constant pool entries between BB#189 and BB#323 to be moved after constant pool entry cp#136, because they are now out of range, and this in turn causes cp#136 to be out of range again.
> 
> BB#324: derived from LLVM BB %bb35
>     Predecessors according to CFG: BB#187
> 	%D0<def> = VORRd %D9, %D9, pred:14, pred:%noreg, %S18<imp-use>
> 	t2B <BB#461>, pred:14, pred:%noreg
>     Successors according to CFG: BB#461
> 
> BB#325: Align 2 (4 bytes)
> 	CONSTPOOL_ENTRY 480, <cp#136>, 4
> 
> ...
> 
> 
> BB#460: Align 2 (4 bytes)
> 	CONSTPOOL_ENTRY 615, <cp#156>, 4
> 
> BB#461: derived from LLVM BB %bb35
>     Predecessors according to CFG: BB#324
> 	%S6<def> = VLDRS <cp#480>, 0, pred:14, pred:%noreg, %D3<imp-def>; mem:LD4[ConstantPool]
> 	%D5<def> = VLDRD <cp#481>, 0, pred:14, pred:%noreg; mem:LD8[ConstantPool]
> 	t2STRi12 %R1<kill>, %R10, 0, pred:14, pred:%noreg; mem:ST4[%arg14]
> 	t2B <BB#479>, pred:14, pred:%noreg
>     Successors according to CFG: BB#479
> <constant-island1.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141016/98d5365d/attachment.html>


More information about the llvm-commits mailing list