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

[Bob Wilson]

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.

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
>