[llvm-commits] [llvm] r84172 - /llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp

[Jim Grosbach]

On Oct 15, 2009, at 9:37 AM, Bob Wilson wrote:

>
> On Oct 15, 2009, at 9:31 AM, Jim Grosbach wrote:
>
>>
>> On Oct 14, 2009, at 10:10 PM, Bob Wilson wrote:
>>
>>> Author: bwilson
>>> Date: Thu Oct 15 00:10:36 2009
>>> New Revision: 84172
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=84172&view=rev
>>> Log:
>>> Fix another problem with ARM constant pools.  Radar 7303551.
>>> When ARMConstantIslandPass cannot find any good locations (i.e.,  
>>> "water") to
>>> place constants, it falls back to inserting unconditional branches  
>>> to make a
>>> place to put them.  My recent change exposed a problem in this  
>>> area.  We may
>>> sometimes append to the same block more than one unconditional  
>>> branch.  The
>>> symptoms of this are that the generated assembly has a branch to  
>>> an undefined
>>> label and running llc with -debug will cause a seg fault.
>>>
>>> This happens more easily since my change to prevent CPEs from  
>>> moving from
>>> lower to higher addresses as the algorithm iterates, but it could  
>>> have
>>> happened before.  The end of the block may be in range for various  
>>> constant
>>> pool references, but the insertion point for new CPEs is not right  
>>> at the end
>>> of the block -- it is at the end of the CPEs that have already  
>>> been placed
>>> at the end of the block.  The insertion point could be out of  
>>> range.  When
>>> that happens, the fallback code will always append another  
>>> unconditional
>>> branch if the end of the block is in range.
>>>
>>> The fix is to only append an unconditional branch if the block  
>>> does not
>>> already end with one.  I also removed a check to see if the  
>>> constant pool load
>>> instruction is at the end of the block, since that is redundant with
>>> checking if the end of the block is in-range.
>>>
>>
>> Doesn't this mean that the new constant pool entry still be out of  
>> range? Or, if it's added at the top of the list rather than the  
>> end, that it could potentially move other items in the pool to be  
>> out of range? Not sure I completely follow.
>
> If it can't add an unconditional branch, the next thing it tries is  
> to split the block.  That will always succeed.

Aha! I misunderstood what you were getting at. This makes much more  
sense. Thanks!

>
> The key is to never go back and reconsider placing a constant in a  
> location that has already been considered*.  Doing so can prevent  
> the pass from terminating.  If you want to discuss the details, it  
> would be easier to do it in person.
>
> * Footnote: ....except that I'm working on another patch that will  
> make that condition slightly more complicated.  Details to follow.
>
>>
>>> There is more to be done here, but I think this fixes the  
>>> immediate problem.
>>>
>>> Modified:
>>>  llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp
>>>
>>> Modified: llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp?rev=84172&r1=84171&r2=84172&view=diff
>>>
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> = 
>>> ====================================================================
>>> --- llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp (original)
>>> +++ llvm/trunk/lib/Target/ARM/ARMConstantIslandPass.cpp Thu Oct 15  
>>> 00:10:36 2009
>>> @@ -1006,14 +1006,12 @@
>>>                              BBSizes[UserMBB->getNumber()];
>>> assert(OffsetOfNextBlock== BBOffsets[UserMBB->getNumber()+1]);
>>>
>>> -  // If the use is at the end of the block, or the end of the block
>>> -  // is within range, make new water there.  (The addition below is
>>> -  // for the unconditional branch we will be adding:  4 bytes on  
>>> ARM + Thumb2,
>>> -  // 2 on Thumb1.  Possible Thumb1 alignment padding is allowed for
>>> +  // If the block does not end in an unconditional branch  
>>> already, and if the
>>> +  // end of the block is within range, make new water there.   
>>> (The addition
>>> +  // below is for the unconditional branch we will be adding: 4  
>>> bytes on ARM +
>>> +  // Thumb2, 2 on Thumb1.  Possible Thumb1 alignment padding is  
>>> allowed for
>>> // inside OffsetIsInRange.
>>> -  // If the block ends in an unconditional branch already, it is  
>>> water,
>>> -  // and is known to be out of range, so we'll always be adding a  
>>> branch.)
>>> -  if (&UserMBB->back() == UserMI ||
>>> +  if (BBHasFallthrough(UserMBB) &&
>>>     OffsetIsInRange(UserOffset, OffsetOfNextBlock + (isThumb1 ? 2:  
>>> 4),
>>>                     U.MaxDisp, U.NegOk, U.IsSoImm)) {
>>>   DEBUG(errs() << "Split at end of block\n");
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>