Page MenuHomePhabricator

[DL] Make vector ABI align bound by element align
Needs ReviewPublic

Authored by beanz on Sep 6 2022, 2:51 PM.

Details

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.

Diff Detail

Event Timeline

beanz created this revision.Sep 6 2022, 2:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 2:51 PM
beanz requested review of this revision.Sep 6 2022, 2:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 6 2022, 2:51 PM

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 added a subscriber: tex3d.Sep 6 2022, 4:08 PM
tex3d added a comment.Sep 6 2022, 4:36 PM

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 updated this revision to Diff 459160.Sep 9 2022, 12:19 PM

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.

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 added a subscriber: arsenm.Sat, Dec 3, 6:28 PM

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 added a comment.Sat, Dec 3, 6:33 PM

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 added a comment.Mon, Dec 5, 11:10 AM

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.

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.

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 added a comment.EditedFri, Dec 9, 2:53 AM

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.