Page MenuHomePhabricator

[LangRef] Remove incorrect vector alignment rules

Authored by frasercrmck on Oct 25 2021, 9:09 AM.



The LangRef incorrectly says that if no exact match is found when
seeking alignment for a vector type, the largest vector type smaller
than the sought-after vector type. This is incorrect as vector types
require an exact match, else they fall back to reporting the natural

The corrected rule was not added in its place, as rules for other types
(e.g., floating-point types) aren't documented.

A unit test was added to demonstrate this.

Diff Detail

Event Timeline

frasercrmck created this revision.Oct 25 2021, 9:09 AM
frasercrmck requested review of this revision.Oct 25 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 9:09 AM

Sorry for the scatter-gun approach to reviewers: I struggled to find the right set. If anyone feels able to review this there's also D112316 which I opened on Friday.

it'd be good for somebody with more background to confirm if this is intended or not




I would just delete everything after this, it's not useful to anybody in the future.

I wouldn't call this "NFC" if it's updating the LangRef

frasercrmck marked 2 inline comments as done.
  • avoid using auto for Expected<DataLayout>
  • fix comment typo
  • chop off bits of comment

I wouldn't call this "NFC" if it's updating the LangRef

I can see that, yeah. I'm happy to defer to your judgement on that. The absence of NFC doesn't hurt as much as a badly-placed NFC.


Yeah, good point; agreed.

frasercrmck retitled this revision from [LangRef][NFC] Remove incorrect vector alignment rules to [LangRef] Remove incorrect vector alignment rules.Oct 25 2021, 9:30 AM

Not sure whether that behavior is intended, but it is indeed the current behavior.

LGTM structurally, but I'll defer approval to someone who knows this bit better.

aeubanks added inline comments.Tue, Nov 30, 9:12 AM

I'm not very familiar with vectors, but does this say that a 128-bit vector is a legal type? Then this wouldn't be testing the langref change.

frasercrmck added inline comments.Tue, Nov 30, 9:31 AM

Not legal as such, it just specifies the ABI alignment of 128-bit vectors. The test is confirming that a 1024-bit vector (<128 x double>) doesn't report the alignment of the largest vector smaller than this vector, as the DataLayout says/said it would. If so we'd expect to see the alignment we'd specified for v128 (16 bytes/128 bits), when in fact we observe that it just returns the natural alignment of the vector: 128 bytes/1024 bits.

Perhaps the examples I'm using are needlessly confusing because I'm using 128 in a few places?

aeubanks accepted this revision.Tue, Nov 30, 9:35 AM
aeubanks added inline comments.

ah I see, yes using something other than 128 in other places would be clearer

This revision is now accepted and ready to land.Tue, Nov 30, 9:35 AM

Choose better numbers to remove confusing use of 128

frasercrmck marked an inline comment as done.Tue, Nov 30, 10:02 AM
frasercrmck added inline comments.

Done, cheers.