LowerByValAttribute: propagate the byval alignment to the local copy - #425
Open
pvelesko wants to merge 2 commits into
Open
LowerByValAttribute: propagate the byval alignment to the local copy#425pvelesko wants to merge 2 commits into
pvelesko wants to merge 2 commits into
Conversation
LowerByValAttribute replaces a byval pointer argument with a local copy.
The copy is created with builder.CreateAlloca(ElTy) and the memcpy uses
DL.getABITypeAlign(ElTy); neither consults the alignment stated in the
`byval(%T) align N` attribute.
For `{[8 x double]}` the ABI alignment is 8, so a parameter declared
`byval align 64` loses the guarantee its callee is entitled to rely on.
byval-explicit-alignment.ll checks both directions: the copy of the
over-aligned argument must come out `align 64`, and the copy of a
parameter with no explicit alignment must keep exactly the alignment it
has today, `align 8` - the preferred alignment CreateAlloca() gives an
aggregate under the default data layout, not its ABI alignment of 4. The
second case is there so that a fix cannot raise one alignment by lowering
the other.
Expected to fail until the attribute's alignment is propagated.
Related: intel#392. This is a second, latent
instance of the same class of bug; the reproducer in that issue takes a
different path (its parameters are readonly, which this pass
short-circuits).
Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
The hidden copy that replaces a byval pointer argument was created with
builder.CreateAlloca(ElTy) and memcpy'd with DL.getABITypeAlign(ElTy),
neither of which looks at the alignment stated in the attribute itself.
For `byval({[8 x double]}) align 64` that gives a copy aligned to 8.
The callee is entitled to rely on the alignment the attribute promises:
downstream codegen folds constant offsets into a bitwise OR on the low
half of the pointer, which is only valid when the low bits are zero. An
under-aligned copy therefore breaks that entitlement.
Raise the alloca to the attribute's alignment via std::max against the
alignment it already has: CreateAlloca() gives the *preferred* alignment
of the element type, which for an aggregate is 8 under the default data
layout even when its ABI alignment is only 4, so the alignment must only
ever be raised, never overwritten with max(ABI, attribute), which would
silently demote every type whose preferred alignment exceeds its ABI
alignment. The memcpy's destination alignment follows the alloca; its
source alignment stays at what is actually known about the incoming
pointer, which is the attribute's alignment when there is one and the
ABI alignment otherwise.
Where the attribute states no alignment, nothing changes.
Fixes the test added in the preceding commit.
Signed-off-by: Paulius Velesko <pvelesko@pglc.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The copy that replaces a byval pointer argument was created with
CreateAlloca(ElTy)and memcpy'd withgetABITypeAlign(ElTy), neither of which consults the alignment stated on the attribute.getParamAlign()is not called anywhere in the pass. Forbyval({[8 x double]}) align 64that yields a copy aligned to 8 while the call still claims 64, and downstream codegen folds constant offsets into a bitwise OR on the low half of the pointer, which is only valid when those bits are zero.Raise the alloca to the attribute's alignment. Raising rather than assigning matters:
CreateAlloca()gives the type's preferred alignment, which for an aggregate can already exceed both the ABI alignment and the attribute, so assigning would demote it. The memcpy destination follows the alloca; its source stays at what is actually known about the incoming pointer.Where the attribute states no alignment, nothing changes. The test checks both directions so a fix cannot raise one alignment by lowering the other.
Related to #392, which reaches a similar misalignment through
PrivateMemoryResolution; the reproducer there has readonly parameters, which this pass skips.Fixes #423