Page MenuHomePhabricator

[IR] GEP: Fix byte-offsets in vectors of overaligned types
AbandonedPublic

Authored by jsilvanus on Nov 30 2022, 10:20 AM.

Details

Reviewers
nikic
efriedma
Summary

Vectors contain their elements tightly packed together without
any padding bytes. If the elements have stricter-than-natural
alignment requirements, then the elements in the vector are smaller
than in for example an array or struct.

This fact was not accounted for when computing byte-offsets of GEPs.
For example, with i16:32 alignment, the byte-offset of the following
GEP was incorrectly computed as 8 bytes, while 4 bytes is correct:

getelementptr <3 x i16>, <3 x i16> *%ptr, i32 0, i32 2

To fix, instead of unconditionally using the elements' AllocSize,
use a new helper getElementSize(..) that returns the inner type's
size if the outer type is a vector, otherwise its AllocSize.

Add a unit test.

Also add an assertion to SROA:
SROA tries to generate "natural" GEPs for a given byte offset if possible,
instead of bitcasting to i8* and applying the offset there.
Add an assertion that such a generated natural GEP results in the correct
byte offset. This assertion would have failed prior to the GEP fix.

Update the LangRef to be more explicit on the layout of vectors of
overaligned elements.

The lit test is new. I'll precommit it if this patch is accepted.

Diff Detail

Event Timeline

jsilvanus created this revision.Nov 30 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 10:20 AM
jsilvanus requested review of this revision.Nov 30 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 10:20 AM

Adding reviewers, based on past changes to GetElementPtrTypeIterator.h.
Please feel free to reject or suggest other reviewers that might have a better background.

nikic added a comment.Dec 5 2022, 3:54 AM

I think the general idea behind this change is correct. Vectors should indeed be tightly packed and ignore ABI alignment of the element type.

However, I'm afraid that the patch in the current form will make the situation worse by introducing inconsistent treatment in different places. We have a lot more code that is repeating basically the same pattern (getAllocSize on the GTI indexed type). BasicAA would be an obvious example, but I see quite a few more uses spread over the codebase from a grep on getTypeAllocSize(GTI.

I think that ideally we would add a gep_offset_iterator which exposes the GEP as an addition of scaled multiplies and constants only and hides the type-related details. This can be used in most places and would give a central place to update the offset calculation logic.

llvm/lib/Transforms/Scalar/SROA.cpp
1542

This entire function is unused if opaque pointers are used, so I don't think it makes much sense to add this assertion.

Thanks for the review.
Your idea of adding a gep_offset_iterator seems plausible. I'll look into that.

As a heads-up notice, it may take a while until I can continue working on this.

jsilvanus abandoned this revision.Thu, Jan 19, 4:40 AM

Abandoning this change in favor of avoiding GEPs into vectors instead of fixing their offsets.

I'll start with avoiding to generate vector GEPs in SROA.
In the future, we may forbid vector GEPs entirely, see https://discourse.llvm.org/t/status-of-geps-into-vectors-of-overaligned-elements/67497.