[LangRef] Remove incorrect vector alignment rules

[frasercrmck]

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.

[frasercrmck]

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.

[aeubanks]

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.

[aeubanks]

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

[frasercrmck]

• avoid using auto for Expected<DataLayout>
• use EXPECT_THAT_EXPECTED rather than EXPECT_THAT_ERROR
• fix comment typo
• chop off bits of comment

[frasercrmck]

In D112463#3084766, @aeubanks wrote:

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.

[woodruffw]

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.

[frasercrmck]

ping

[frasercrmck]

ping, thanks

[aeubanks]

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]

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]

llvm/unittests/IR/DataLayoutTest.cpp

94

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

[frasercrmck]

Choose better numbers to remove confusing use of 128

[frasercrmck]

llvm/unittests/IR/DataLayoutTest.cpp

94

Done, cheers.

[frasercrmck]

ping, thanks

[frasercrmck]

whoops I didn't catch it was already accepted, sorry!

llvm/docs/LangRef.rst

#. If the type sought is an exact match for one of the specifications,
    that specification is used.
 #. If no match is found, and the type sought is an integer type, then
    the smallest integer type that is larger than the bitwidth of the
    sought type is used. If none of the specifications are larger than
    the bitwidth then the largest integer type is used. For example,
    given the default specifications above, the i7 type will use the
    alignment of i8 (next largest) while both i65 and i256 will use the
    alignment of i64 (largest specified).
-#. If no match is found, and the type sought is a vector type, then the
-   largest vector type that is smaller than the sought vector type will
-   be used as a fall back. This happens because <128 x double> can be
-   implemented in terms of 64 <2 x double>, for example.
 
 The function of the data layout string may not be what you expect.
 Notably, this is not a specification from the frontend of what alignment
 the code generator should use.
 
 Instead, if specified, the target data layout is required to match what
 the ultimate *code generator* expects. This string is used by the
 mid-level optimizers to improve code, and this only works if it matches

llvm/unittests/IR/DataLayoutTest.cpp

 //===- ConstantRangeTest.cpp - ConstantRange tests ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 TEST(DataLayoutTest, FunctionPtrAlign) {
   EXPECT_EQ(MaybeAlign(0), DataLayout("").getFunctionPtrAlign());
auto *DefaultGlobal2 = new GlobalVariable(
       *M, Int32, false, GlobalValue::ExternalLinkage, nullptr);
   EXPECT_EQ(DefaultGlobal2->getAddressSpace(), 200u);
   auto *ExplicitGlobal2 = new GlobalVariable(
       *M, Int32, false, GlobalValue::ExternalLinkage, nullptr, "", nullptr,
       GlobalValue::NotThreadLocal, 123);
   EXPECT_EQ(ExplicitGlobal2->getAddressSpace(), 123u);
 }
 
+TEST(DataLayoutTest, VectorAlign) {
+  auto DL = DataLayout::parse("v128:128");
+  EXPECT_THAT_ERROR(DL.takeError(), Succeeded());
+
+  LLVMContext Context;
+  Type *const DoubleTy = Type::getDoubleTy(Context);
+  Type *const V128F64Ty = FixedVectorType::get(DoubleTy, 128);
+
+  // The alignment for a vector type larger than any specified vector type uses
+  // the natual alignment as a fallback. This is contrary to old LangRef
+  // documentation which claimed it would use the largest vector type smaller
+  // than the one sought after. In this case, it would have been Align(16).
+  EXPECT_EQ(Align(128 * 8), DL->getABITypeAlign(V128F64Ty));
+  EXPECT_EQ(Align(128 * 8), DL->getPrefTypeAlign(V128F64Ty));
+}
+
 } // anonymous namespace