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

[Akira Hatanaka]

Thanks Bob. Committed in r220015.

On Thu, Oct 16, 2014 at 4:43 PM, Bob Wilson <bob.wilson at apple.com> wrote:

> 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/c5a776d1/attachment.html>