Page MenuHomePhabricator

[LangRef] Remove incorrect vector alignment rules
AcceptedPublic

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

Details

Summary

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
alignment.

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

llvm/unittests/IR/DataLayoutTest.cpp
102

natural

102

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>
  • use EXPECT_THAT_EXPECTED rather than EXPECT_THAT_ERROR
  • 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.

llvm/unittests/IR/DataLayoutTest.cpp
102

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
llvm/unittests/IR/DataLayoutTest.cpp
94

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
llvm/unittests/IR/DataLayoutTest.cpp
94

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.
llvm/unittests/IR/DataLayoutTest.cpp
94

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.
llvm/unittests/IR/DataLayoutTest.cpp
94

Done, cheers.