[DL] Make vector ABI align bound by element align

[beanz]

Summary

If a data layout specifies a vector alignment that is less than the
alignment of the element type, prefer the element alignment.

This alters the vector ABI alignment so that vector elements are always
aligned appropriately for their type.

This oddity of data layout doesn't seem to happen in any existing
backends, but does in DirectX where vector alignment has a lower bound
of the element type. For example < 4 x float > requires 32-bit
alignment, and < 2 x double > requires 64.

[efriedma]

Whatever we do here should be documented in LangRef.

I'm a little wary of making assumptions like this implicitly. I think I'd prefer to make the datalayout string explicitly request this behavior.

llvm/include/llvm/Support/Alignment.h

218 ↗

(On Diff #458288)

Can you not just use std::max?

[tex3d]

I don't see why we would want a different behavior here between ABI and preferred alignments.

llvm/lib/IR/DataLayout.cpp

813

Shouldn't this capture the preferred alignment value for use in the maxAlignment, or return if not vector? Or is this mechanism intended to only apply to ABI alignment and not preferred alignment?

[beanz]

Updating based on review feedbac.

• Apply the behavior change to both ABI and Preferred alignments.
• Gate the change on a data layout variable (V).
• Update LangRef.rst documenting the new variable.

[jsilvanus]

This alters the vector ABI alignment so that vector elements are always aligned appropriately for their type.

How should this behave on overaligned element types? For example, with a i16:32 data layout (as in the old DXIL data layout),
the second element in <3 x i16> will not be 32-bit aligned, even if the vector is.

[arsenm]

Can dxil vectors be translated as arrays instead? Feels like someone interpreted vectors as directly mapping to a language language rather than a feature that doesn’t fit its needs.

[arsenm]

I have wanted to allow preferred alignments to be lower than the ABI alignments. Can we get per-type alignment specification instead of a global modifier?

[beanz]

In D133379#3962893, @jsilvanus wrote:

How should this behave on overaligned element types? For example, with a i16:32 data layout (as in the old DXIL data layout),
the second element in <3 x i16> will not be 32-bit aligned, even if the vector is.

16-bit types aren't supported with the old data layout at all. If you enable 16-bit types when compiling HLSL you get the new data layout which aligns 16-bit types to 16-bit boundaries. The only exception to that is some oddities around how buffers are packed, but that doesn't impact the data layout.

In D133379#3968986, @arsenm wrote:

Can dxil vectors be translated as arrays instead? Feels like someone interpreted vectors as directly mapping to a language language rather than a feature that doesn’t fit its needs.

DXIL is a shipped binary interface using IR vectors in this way. The current compiler does this without capturing the behavior in the data layout, which is something we'd like to change as we upstream the work.

In D133379#3968993, @arsenm wrote:

I have wanted to allow preferred alignments to be lower than the ABI alignments. Can we get per-type alignment specification instead of a global modifier?

Per-type specification makes sense, I can update the patch later this week.

[jsilvanus]

In D133379#3971623, @beanz wrote:

In D133379#3962893, @jsilvanus wrote:

How should this behave on overaligned element types? For example, with a i16:32 data layout (as in the old DXIL data layout),
the second element in <3 x i16> will not be 32-bit aligned, even if the vector is.

16-bit types aren't supported with the old data layout at all. If you enable 16-bit types when compiling HLSL you get the new data layout which aligns 16-bit types to 16-bit boundaries. The only exception to that is some oddities around how buffers are packed, but that doesn't impact the data layout.

Sure -- but still the behavior on such cases should be clear, even if it is not relevant for DXIL. I just stumbled over the formulation

This alters the vector ABI alignment so that vector elements are always aligned appropriately for their type.

that is promising too much in this case.

[beanz]

In D133379#3984033, @jsilvanus wrote:

Sure -- but still the behavior on such cases should be clear, even if it is not relevant for DXIL. I just stumbled over the formulation

I don't disagree. That wasn't the problem I was trying to solve with this change.

This alters the vector ABI alignment so that vector elements are always aligned appropriately for their type.

that is promising too much in this case.

Fair. That is probably an overly stated comment.

llvm/docs/LangRef.rst

 ``E``
     Specifies that the target lays out data in big-endian form. That is,
     the bits with the most significance have the lowest address
     location.
 ``e``
     Specifies that the target lays out data in little-endian form. That
     is, the bits with the least significance have the lowest address
     location.
+``V``
+    Specifies that when resolving vector alignments, if the vector element
+    type's alignment is greater than the vector alignment, prefer the element
+    type's alignment.
 ``S<size>``
     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. If omitted, the natural stack
     alignment defaults to "unspecified", which does not prevent any
     alignment promotions.
 ``P<address space>``

llvm/include/llvm/IR/DataLayout.h

enum class FunctionPtrAlignType {
     /// The function pointer alignment is independent of the function alignment.
     Independent,
     /// The function pointer alignment is a multiple of the function alignment.
     MultipleOfFunctionAlign,
   };
 private:
   /// Defaults to false.
   bool BigEndian;
+  bool ConsidersElementAlignment;
 
   unsigned AllocaAddrSpace;
   MaybeAlign StackNaturalAlign;
   unsigned ProgramAddrSpace;
   unsigned DefaultGlobalsAddrSpace;
 
   MaybeAlign FunctionPtrAlign;
   FunctionPtrAlignType TheFunctionPtrAlignType;
public:
   DataLayout(const DataLayout &DL) { *this = DL; }
 
   ~DataLayout(); // Not virtual, do not subclass this class
 
   DataLayout &operator=(const DataLayout &DL) {
     clear();
     StringRepresentation = DL.StringRepresentation;
     BigEndian = DL.isBigEndian();
+    ConsidersElementAlignment = DL.considersElementAlignment();
     AllocaAddrSpace = DL.AllocaAddrSpace;
     StackNaturalAlign = DL.StackNaturalAlign;
     FunctionPtrAlign = DL.FunctionPtrAlign;
     TheFunctionPtrAlignType = DL.TheFunctionPtrAlignType;
     ProgramAddrSpace = DL.ProgramAddrSpace;
     DefaultGlobalsAddrSpace = DL.DefaultGlobalsAddrSpace;
     ManglingMode = DL.ManglingMode;
     LegalIntWidths = DL.LegalIntWidths;
public:
   /// Parse a data layout string and return the layout. Return an error
   /// description on failure.
   static Expected<DataLayout> parse(StringRef LayoutDescription);
 
   /// Layout endianness...
   bool isLittleEndian() const { return !BigEndian; }
   bool isBigEndian() const { return BigEndian; }
 
+  /// Returns true if the data layout considers vector element type alignment
+  /// when determining vector alignments.
+  bool considersElementAlignment() const { return ConsidersElementAlignment; }
+
   /// Returns the string representation of the DataLayout.
   ///
   /// This representation is in the same format accepted by the string
   /// constructor above. This should not be used to compare two DataLayout as
   /// different string can represent the same layout.
   const std::string &getStringRepresentation() const {
     return StringRepresentation;
   }

llvm/lib/IR/DataLayout.cpp

static const LayoutAlignElem DefaultAlignments[] = {
     {AGGREGATE_ALIGN, 0, Align(1), Align(8)}   // struct
 };
 
 void DataLayout::reset(StringRef Desc) {
   clear();
 
   LayoutMap = nullptr;
   BigEndian = false;
+  ConsidersElementAlignment = false;
   AllocaAddrSpace = 0;
   StackNaturalAlign.reset();
   ProgramAddrSpace = 0;
   DefaultGlobalsAddrSpace = 0;
   FunctionPtrAlign.reset();
   TheFunctionPtrAlignType = FunctionPtrAlignType::Independent;
   ManglingMode = MM_None;
   NonIntegralAddressSpaces.clear();
case 'm':
       case 'x':
         ManglingMode = MM_WinCOFFX86;
         break;
       case 'a':
         ManglingMode = MM_XCOFF;
         break;
       }
       break;
+    case 'V':
+      ConsidersElementAlignment = true;
+      break;
     default:
       return reportError("Unknown specifier in datalayout string");
       break;
     }
   }
 
   return Error::success();
 }
 
 DataLayout::DataLayout(const Module *M) {
   init(M);
 }
 
 void DataLayout::init(const Module *M) { *this = M->getDataLayout(); }
 
 bool DataLayout::operator==(const DataLayout &Other) const {
   bool Ret = BigEndian == Other.BigEndian &&
+             ConsidersElementAlignment == Other.ConsidersElementAlignment &&
              AllocaAddrSpace == Other.AllocaAddrSpace &&
              StackNaturalAlign == Other.StackNaturalAlign &&
              ProgramAddrSpace == Other.ProgramAddrSpace &&
              DefaultGlobalsAddrSpace == Other.DefaultGlobalsAddrSpace &&
              FunctionPtrAlign == Other.FunctionPtrAlign &&
              TheFunctionPtrAlignType == Other.TheFunctionPtrAlignType &&
              ManglingMode == Other.ManglingMode &&
              LegalIntWidths == Other.LegalIntWidths &&
case Type::X86_FP80TyID: {
     return Align(PowerOf2Ceil(BitWidth / 8));
   }
   case Type::X86_MMXTyID:
   case Type::FixedVectorTyID:
   case Type::ScalableVectorTyID: {
     unsigned BitWidth = getTypeSizeInBits(Ty).getKnownMinSize();
     auto I = findAlignmentLowerBound(VECTOR_ALIGN, BitWidth);
     if (I != Alignments.end() && I->AlignType == VECTOR_ALIGN &&
-        I->TypeBitWidth == BitWidth)
-      return abi_or_pref ? I->ABIAlign : I->PrefAlign;
+        I->TypeBitWidth == BitWidth) {
+      Align VectorAlign = abi_or_pref ? I->ABIAlign : I->PrefAlign;
+
+      // If we have a specified vector alignment the vector ABI should be the
+      // larger of the vector alignment and the element type's alignment.
+      if (considersElementAlignment())
+        if (auto VTy = dyn_cast<VectorType>(Ty))
+          return std::max(VectorAlign,
+                          getAlignment(VTy->getElementType(), abi_or_pref));
+      return VectorAlign;
+    }
 
     // By default, use natural alignment for vector types. This is consistent
     // with what clang and llvm-gcc do.
     //
     // We're only calculating a natural alignment, so it doesn't have to be
     // based on the full size for scalable vectors. Using the minimum element
     // count should be enough here.
     return Align(PowerOf2Ceil(getTypeStoreSize(Ty).getKnownMinSize()));

llvm/unittests/IR/DataLayoutTest.cpp

TEST(DataLayoutTest, VectorAlign) {
   Type *const V8F32Ty = FixedVectorType::get(FloatTy, 8);
 
   // The alignment for a vector type larger than any specified vector type uses
   // the natural alignment as a fallback.
   EXPECT_EQ(Align(4 * 8), DL->getABITypeAlign(V8F32Ty));
   EXPECT_EQ(Align(4 * 8), DL->getPrefTypeAlign(V8F32Ty));
 }
 
+TEST(DataLayoutTest, VectorAlignElement) {
+  LLVMContext Context;
+  Type *const FloatTy = Type::getFloatTy(Context);
+  Type *const V4F32 = FixedVectorType::get(FloatTy, 4);
+  Type *const HalfTy = Type::getHalfTy(Context);
+  Type *const V4F16 = FixedVectorType::get(HalfTy, 4);
+
+  {
+    Expected<DataLayout> DL =
+        DataLayout::parse("V-f16:16:16-f32:32:32-v64:8:8-v128:8:8");
+    EXPECT_THAT_EXPECTED(DL, Succeeded());
+    EXPECT_TRUE(DL->considersElementAlignment());
+
+    // The alignment for a vector type larger than any specified vector type
+    // uses the natural alignment as a fallback.
+    EXPECT_EQ(DL->getABITypeAlign(FloatTy), DL->getABITypeAlign(V4F32));
+    EXPECT_EQ(DL->getABITypeAlign(HalfTy), DL->getABITypeAlign(V4F16));
+
+    EXPECT_EQ(DL->getPrefTypeAlign(FloatTy), DL->getPrefTypeAlign(V4F32));
+    EXPECT_EQ(DL->getPrefTypeAlign(HalfTy), DL->getPrefTypeAlign(V4F16));
+  }
+
+  {
+    Expected<DataLayout> DL = DataLayout::parse("V-v64:8:8-v128:8:8");
+    EXPECT_THAT_EXPECTED(DL, Succeeded());
+    EXPECT_TRUE(DL->considersElementAlignment());
+
+    // The alignment for a vector type larger than any specified vector type
+    // uses the natural alignment as a fallback.
+    EXPECT_EQ(DL->getABITypeAlign(FloatTy), DL->getABITypeAlign(V4F32));
+    EXPECT_EQ(DL->getABITypeAlign(HalfTy), DL->getABITypeAlign(V4F16));
+
+    EXPECT_EQ(DL->getPrefTypeAlign(FloatTy), DL->getPrefTypeAlign(V4F32));
+    EXPECT_EQ(DL->getPrefTypeAlign(HalfTy), DL->getPrefTypeAlign(V4F16));
+  }
+
+  {
+    Expected<DataLayout> DL =
+        DataLayout::parse("f16:16:16-f32:32:32-v64:8:8-v128:8:8");
+    EXPECT_THAT_EXPECTED(DL, Succeeded());
+    EXPECT_FALSE(DL->considersElementAlignment());
+
+    // Without the `V` option, the vector alignments from the data layout are
+    // used regardless of the element alignments.
+    EXPECT_EQ(Align(1), DL->getABITypeAlign(V4F32));
+    EXPECT_EQ(Align(1), DL->getABITypeAlign(V4F16));
+
+    EXPECT_EQ(Align(1), DL->getPrefTypeAlign(V4F32));
+    EXPECT_EQ(Align(1), DL->getPrefTypeAlign(V4F16));
+  }
+}
+
 } // anonymous namespace