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

[Jim Grosbach]

On Nov 23, 2009, at 9:44 AM, Chris Lattner wrote:

> On Nov 22, 2009, at 10:28 AM, Jim Grosbach wrote:
>>> Are you sure that this is the right thing to do?  GCC inline asm  
>>> precisely specifies how the size of an inline asm is computed:
>>> http://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm  
>>> (6.39.1)
>>>
>>> If some inline asm isn't working with this algorithm, then the  
>>> inline asm is wrong and should be fixed.
>>
>>
>> Yes, I believe this is the correct approach.
>>
>> The algorithm described is, "The estimate is formed by counting the  
>> number of statements in the pattern of the asm and multiplying that  
>> by the length of the longest instruction on that processor." This  
>> is exactly what we do, and gets us close, but it is still an  
>> estimate. To do better, we'll need to actually parse the assembly  
>> code to determine whether it's a 16 or a 32 bit instruction. In  
>> Thumb2, that's a non-trivial thing, and I suspect we don't want to  
>> go down that path, at least right now.
>
> No, this isn't true at all.  This is a statement of how the language  
> extension (in this case, gnu assembly) works.  If the "estimate" is  
> incorrect for a piece of code, then the asm is incorrect, not the  
> compiler.
>

I think we're talking past one another here but get back on the same  
page in your next comment.

>> What's happening here is that the inline asm is actually shorter  
>> than the estimate since it's a 16-bit instruction, and is also not  
>> located between the constant pool reference and the constant pool  
>> entry. Since it's different in size than what the estimate  
>> believes, our calculations for when alignment padding will be  
>> inserted are now off, leading to problems.
>
> Ah, this is a completely different problem from what I thought.  I  
> thought we were incorrectly under-estimating the size of an asm, not  
> overestimating it.  If the code is expecting to get the exact size  
> of the asm, it is definitely "doomed to failure" :).
>

Yes, that's exactly the problem: the code is expecting the exact size  
(in bytes) of the asm, is over-estimating it, and is falling over as a  
result. You're absolutely right that if we were underestimating the  
size of the inline asm, then the problem would be a flaw in that  
algorithm.

>   However, if you just need alignment, can you just not insert  
> "alignment padding" and instead insert a .align directive after the  
> asm?


We already use an .align directive to actually insert the padding,  
which is good. Otherwise we'd likely be having far more failures of  
other sorts. The constant island pool pass thinks it knows for sure  
what that directive will do (that is, when it will and won't insert  
padding bytes, and how many), and takes that into account when  
calculating distances between instructions. In the presence of inline  
asm, it can get it wrong and mistakenly think padding has not been  
added when it actually has, resulting in the out of range error since  
the target constant pool entry is actually further away than the  
compiler thought.

>> This patch adjusts those padding calculations to know that in the  
>> presence of inline assembly, we're dealing with an estimate and  
>> therefore can't make assumptions about alignment padding.
>
> Ah ok, so this just causes extra .align directives to be emitted, it  
> doesn't make wildly conservative assumptions?  If so, sounds great! :)

Close. We're not inserting any additional directives, but rather  
better understanding the ones we're already emitting. It causes us to  
correctly understand that when we have inline asm, we don't know for  
sure when an .align directive will cause padding to be inserted. We  
know when it may be inserted, and track that instead. Note that in the  
absence of inline asm, we continue to calculate precisely. I believe  
this is well on the side of prudently rather than wildly conservative.

Thanks,
   Jim