[llvm-commits] [llvm] r141599 - in /llvm/trunk: docs/LangRef.html include/llvm/Target/TargetData.h lib/Target/ARM/ARMTargetMachine.cpp lib/Target/TargetData.cpp lib/Target/X86/README-SSE.txt lib/Target/X86/README.txt lib/Target/X86/X86TargetMachine.cpp lib/Transforms/Utils/Local.cpp

[Lang Hames]

Hi Duncan,

I've updated the docs to reflect the proper default/behavior. Thanks for catching that.

Allowing alignment promotion where the alignment data is unspecified means that this patch will not have any impact (compared to existing behavior) on platforms that don't specify an alignment. I think it's safer to have a "no impact" default, and let target maintainers who want to avoid dynamic stack realignment specify a value explicitly, rather than giving a default that won't be sensible for all platforms.

The stack alignment is held in bytes for consistency with the pointer sizes and alignments. It seems to me like all of these should really be held in bits, but I'm sticking to consistency until I have a chance to discuss it with people who know more about TargetData's design than me.

Likewise the simple /=8 operation is consistent with how we treat pointer sizes/alignments, but looks totally unsafe. I'll be looking to fix this shortly.

Cheers,
Lang.


On Oct 11, 2011, at 12:36 AM, Duncan Sands wrote:

> Hi Lang, here you say that the default is zero:
> 
>> The natural stack alignment is set in target data strings via the "S<size>"
>> option. Size is in bits and must be a multiple of 8. The natural stack alignment
>> defaults to "unspecified" (represented by a zero value),
> 
> while here
> 
> =================================
>> --- llvm/trunk/docs/LangRef.html (original)
>> +++ llvm/trunk/docs/LangRef.html Mon Oct 10 18:42:08 2011
>> @@ -1319,6 +1319,12 @@
>>        the bits with the least significance have the lowest address
>>        location.</dd>
>> 
>> +<dt><tt>S<i>size</i></tt></dt>
>> +<dd>Specifies the natural alignment of the stack in bits. Alignment promotion
>> +      of stack variables is limited to the natural stack alignment to avoid
>> +      dynamic stack realignment. The stack alignment must be a multiple of
>> +      8-bits, and currently defaults to 128 bits if unspecified.</dd>
> 
> you say that the default is 128.
> 
>> @@ -163,6 +164,11 @@
>>      return !isLegalInteger(Width);
>>    }
>> 
>> +  /// Returns true if the given alignment exceeds the natural stack alignment.
>> +  bool exceedsNaturalStackAlignment(unsigned Align) const {
>> +    return (StackNaturalAlign != 0)&&  (Align>  StackNaturalAlign);
>> +  }
> 
> Is it really a good idea to do the transform if the stack alignment is unknown?
> 
>> --- llvm/trunk/lib/Target/TargetData.cpp (original)
>> +++ llvm/trunk/lib/Target/TargetData.cpp Mon Oct 10 18:42:08 2011
>> @@ -218,7 +219,12 @@
>>          Token = Split.second;
>>        } while (!Specifier.empty() || !Token.empty());
>>        break;
>> -
>> +    case 'S': // Stack natural alignment.
>> +      StackNaturalAlign = getInt(Specifier.substr(1));
>> +      StackNaturalAlign /= 8;
>> +      // FIXME: Should we really be truncating these alingments and
>> +      // sizes silently?
>> +      break;
> 
> Why don't you keep it in bits?  Also, will the above code accept a negative
> stack alignment?
> 
> Also: "these alingments" -> "alignments"
> 
>> --- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/Local.cpp Mon Oct 10 18:42:08 2011
>> @@ -721,10 +721,14 @@
>>  /// their preferred alignment from the beginning.
>>  ///
>>  static unsigned enforceKnownAlignment(Value *V, unsigned Align,
>> -                                      unsigned PrefAlign) {
>> +                                      unsigned PrefAlign, const TargetData *TD) {
>>    V = V->stripPointerCasts();
>> 
>>    if (AllocaInst *AI = dyn_cast<AllocaInst>(V)) {
>> +    // If the preferred alignment is greater than the natural stack alignment
>> +    // then don't round up. This avoids dynamic stack realignment.
>> +    if (TD&&  TD->exceedsNaturalStackAlignment(PrefAlign))
>> +      return Align;
> 
> Is it really a good idea to increase the alignment if there is no target data?
> 
> Ciao, Duncan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits