[mlir] Support alignment in LLVM dialect GlobalOp

[dpotop]

Summary

First step in adding alignment as an attribute to MLIR global definitions. Alignment can be specified for global objects in LLVM IR. It can also be specified as a named attribute in the LLVMIR dialect of MLIR. However, this attribute has no standing and is discarded during translation from MLIR to LLVM IR. This patch does two things: First, it adds the attribute to the syntax of the llvm.mlir.global operation, and by doing this it also adds accessors and verifications. The syntax is "align=XX" (with XX being an integer), placed right after the value of the operation. Second, it allows transforming this operation to and from LLVM IR. It is checked whether the value is an integer power of 2.

[rriddle]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

435

nit: Drop all of the mlir::, you are already in the mlir namespace.

435

MLIR uses camelCase variable names.

436

Can you run clang-format?

438

Is the template necessary here?

439

Drop the llvm:: here, APInt is exported in the mlir namespace.

439–441

I think getUInt currently asserts that the type is unsigned, though I don't think it should.

[mehdi_amini]

Can you also add test (and support if needed) in the other direction? (LLVM IR->MLIR)

(also see https://mlir.llvm.org/getting_started/Contributing/#commit-messages for the commit message format)

[dpotop]

Le mer. 28 avr. 2021 à 23:52, Mehdi AMINI via Phabricator <
[e-mail]> a écrit :

mehdi_amini added a comment.

Can you also add test (and support if needed) in the other direction?
(LLVM IR->MLIR)

I was not aware that there is a way to go LLVM IR->MLIR. Is this
"mlir-translate --import-llvm"?
I will try to do it (why is this important?).

(also see
https://mlir.llvm.org/getting_started/Contributing/#commit-messages for
the commit message format)

Thanks. I noted the bad format.

D.

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D101492/new/

https://reviews.llvm.org/D101492

[mehdi_amini]

You also miss a test in mlir/test/Target/LLVMIR/llvmir.mlir ; the test you modified is only MLIR->MLIR and does not exercise the translation.

For LLVM IR to MLIR the test is mlir/test/Target/LLVMIR/import.ll

[bondhugula]

Can you please separate the commit title from the summary?

[dpotop]

Corrections and additions to D101492: adding alignment to the LLVMIR dialect

I have made the required corrections and applied clang-format.
I have also added the round-trip LLVMIR->MLIR and the tests.

[ftynse]

Could you please specify the alignment in the definition of the op, similarly to how it's done on, e.g., AllocaOp - https://github.com/llvm/llvm-project/blob/9363aa90bfe6f73df105799abc55bb74d4f186bf/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td#L294. Having random unverified attributes attached to an operation isn't really robust.

[dpotop]

Support alignment in LLVM dialect GlobalOp - add dedicated syntax, accessors

I have added a dedicated syntax to the alignment attribute,
and dedicated accessor routines.
This resulted in changes to multiple files.

[dpotop]

Correcting formatting issues signalled by clang-tidy.

[mehdi_amini]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

441

Nit: avoid duplicating isa+cast, instead the usual idiom is:

IntegerAttr alignAttr = rawAlignAttr.dyn_cast<IntegerAttr>();
if (!alignAttr) 
  return emitError(op.getLoc(),
                               alignment constant should be an integer");

[mehdi_amini]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

1031

The doc does not match the implementation.

Do we need these two method by the way? Why aren't the usual ODS generate accessors enough?

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

441

Also since this is part of the verifier, you don't even need to check. The fast that it is define in ODS should give you a direct accessor: llvm::Optional<uint64_t> align();. on the GlobalOp
(and also a mlir::IntegerAttr alignAttr() accessor).

[ftynse]

mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp

143

Nit: the usual syntax is /*align=*/. Also, you shouldn't be needing to pass null objects for trailing optional attributes.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

910

Bikeshedding: other ops in this dialect use $alignment, I'd prefer this to be consistent and also use $alignment.

987–988

We probably want a verifier for this.

1016–1018

You can use CArg<"Attribute", "Attribute()">:$align here to give the generated argument the default value. This will let you clean up all the places that are passing Attribute() and don't care about alignment.

1031

Nit: this comment doesn't match what the function do, i.e., they don't return an _attribute_.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1290–1292

Other ops in this dialect use the generic attribute dictionary syntax to specify alignment, I think this can do the same and not bother with custom syntax.

1575–1576

Parser should already report an error if any of the calls to parser.parse* fails. We usually just return failure() here and don't emit a second error.

Also, we usually have 100% coverage for user-visible error messages. There is a test suffixed with "invalid" that checks them. (I expect this message, if not the surrounding code, to disappear, so you don't need to add the test).

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

435

Now that alignment attribute is declared in the op spec, you can use op.align(). This is preferred over stringly attribute access.

437–439

There's no need to check this or to emit an error, the generated verifier will have ensured that valid ops have the integer attribute.

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

1016–1018

Actually, on a second thought, consider taking CArg<"unsigned", "0"> and create the attribute in the builder body. This way, the caller doesn't need to bother with creating the attribute and cannot accidentally give you one of a wrong type.

[dpotop]

Code corrections and improvements in response to comments.

[mehdi_amini]

Still LGTM :)
(but you should likely wait for Alex to confirm)

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1293

(nit: remove spurious braces in such cases)

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

438

(here as well)

[rriddle]

Can you run clang-format?

mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp

143

Missed this here? Same comment as Ch6.

mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp

41

Same here.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

2305

and here.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1577

There is a helper function in LLVM for this, named something like IsPowerOf2_32.

2364

Drop the space after the comment.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

479

nit: Comments should be proper sentences (punctuation, etc.)

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

434

Same here.

435

nit: Spell out auto here.

[dpotop]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

910

I have a problem with this, because the IR keyword is "align". Then, one cannot use both the native syntax (with "align") and the general-purpose attribute syntax (with "alignment"). I find this useful. BTW, my code is not the only one where "align" is used. There are some operations (such as llvm.intr.coro.id) which use "align" instead of "alignment".

1016–1018

It was a choice to write all the "Attribute()" manually. I already modified large bases of code, and *not* having default values is helpful, because you have to review all points of use. Let me know if you require this change, or if it's optional.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1290–1292

I don't know how to use the dictionary - I have tried using getAlignAttrName(), and it is not defined, so I guess I should put the name into the dictionary somehow. And since other attributes are left textual...

[dpotop]

Further fancy formatting.

[dpotop]

Lots of "done" comments.

[ftynse]

LGTM with my comments addressed.

Please note that I will change ops in the LLVM dialect to be more consistent if this becomes necessary, I regularly touch this dialect.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

910

I am fine with anything as long as it's consistent. There's one operation that uses $align, and does so for an _operand_ rather than attribute which could justify the difference, and several that use $alignment. I would have gone with the majority rule, or with the similarity rule (how likely one is going to try transferring the "style" of operation from one memory operation to another vs. from coroutine operation to memory operation?)

I won't block you on this because I strongly dislike bikeshedding, but I would be happier with a more consistent naming scheme.

1016–1018

This choice arguably makes life harder for everybody who will want to construct the op in the future. Yes, you got to review all points of use thanks to it. Now this change has served its purpose (you have reviewed all uses) and is no longer justified.

In any case, I will insist on at least changing it to not be generic "Attribute". Please use an unsigned instead.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1290–1292

Just rely on the printOptionalAttrDict before and drop the custom printing/parsing code. MLIR supports an open set of attribute and will print them in the dictionary unless you instruct it not to, which you currently do by putting "align" in the list below. If you don't, it will be printed as llvm.mlir.global @name(...) { align = 32 }.

[dpotop]

No problem for the other modifications, I'll implement them.
However, I have a problem with the following one.

Le jeu. 29 avr. 2021 à 23:38, Alex Zinenko via Phabricator <
[e-mail]> a écrit :

Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:1277-1279
+ if (op.hasAlign()) {
+ p << " align=" << op.getAlign() << " ";

+ }

dpotop wrote:

ftynse wrote:

Other ops in this dialect use the generic attribute dictionary syntax

to specify alignment, I think this can do the same and not bother with
custom syntax.

I don't know how to use the dictionary - I have tried using

getAlignAttrName(), and it is not defined, so I guess I should put the name
into the dictionary somehow. And since other attributes are left textual...
Just rely on the printOptionalAttrDict before and drop the custom
printing/parsing code. MLIR supports an open set of attribute and will
print them in the dictionary unless you instruct it not to, which you
currently do by putting "align" in the list below. If you don't, it will be
printed as llvm.mlir.global @name(...) { align = 32 }.

Is this accepted in MLIR, to have for one attribute a dedicated accessor but
general-purpose syntax/printing/parsing? The general-purpose approach is
what I had done in the beginning. Maybe it deserved some verifications, but
these did not require significant changes in my initial code. The dedicated syntax is
what I implemented yesterday. But a mix of the two just seems off. I'll do it if
you maintain your request, but it just seems off. I'd prefer understanding how to
correctly use the dictionary instead. :)

Dumitru

[ftynse]

I would think that I have enough experience in MLIR and in the LLVM dialect in particular to expect my requests to be interpreted as something that is not only accepted in this part of the codebase but indeed highly recommended. I am still the largest contributor to the file you are editing, https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td...

Yes, it is perfectly fine not to have custom syntax for attributes defined in the operation. So is not having custom syntax at all for the operation. There is absolutely no implied connection whatsoever between how the custom syntax looks like and the API provided on the operation. The general pattern, although it may vary between dialects, is to consider supporting custom syntax for mandatory or frequently used attributes, and keep everything else in the dictionary syntax. Alignment is not mandatory, and doesn't seem that frequently used if we survived without it for over a year that this op exists. The problem you are creating by insisting on not following what the rest of the dialect does is as follows. One knows that to set alignment in memory operations one has to do something like llvm.load %address { alignment = 32 } : !type. This sounds logical to try doing exactly the same on a global llvm.global @name(initializer) { alignment = 32 } : !type. It will fail because this code requires one to use align=32 instead. This kind of small things make the programming interface feel unintuitive if not hostile, and that's why I care about consistency. Now, I am sensitive to your argument that LLVM IR uses align and not alignment and my consistency argument can also apply. All ops should use align to be consistent with LLVM IR and with each other. But let's have the same syntax even if different attribute name at the very least!

[dpotop]

Further corrections to support alignment in LLVMIR dialect GlobalOp

Corrected the syntax to only use the standard form for the
alignment attribute (no dedicated syntax) and to use the
attribute name "alignment", as opposed to "align" or "llvm.align".
Also updated the test cases and added a new one that checks that
alignment is verified.

[ftynse]

Thanks!

[dpotop]

Support alignment in LLVM dialect GlobalOp

Corrected test case.

[dpotop]

Thank you for the help!
Now I'll be moving to the memref dialect to add the attribute to the global
operation, and then complete the lowering to LLVMIR.
Dumitru

Le mar. 11 mai 2021 à 17:42, Alex Zinenko via Phabricator <
[e-mail]> a écrit :

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

Thanks!

Repository:

rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D101492/new/

https://reviews.llvm.org/D101492

[ftynse]

I reformatted your commit before landing.

mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp

static Value getOrCreateGlobalString(Location loc, OpBuilder &builder,
     LLVM::GlobalOp global;
     if (!(global = module.lookupSymbol<LLVM::GlobalOp>(name))) {
       OpBuilder::InsertionGuard insertGuard(builder);
       builder.setInsertionPointToStart(module.getBody());
       auto type = LLVM::LLVMArrayType::get(
           IntegerType::get(builder.getContext(), 8), value.size());
       global = builder.create<LLVM::GlobalOp>(loc, type, /*isConstant=*/true,
                                               LLVM::Linkage::Internal, name,
-                                              builder.getStringAttr(value));
+                                              builder.getStringAttr(value),
+                                              /*alignment=*/0);
     }
 
     // Get the pointer to the first character in the global string.
     Value globalPtr = builder.create<LLVM::AddressOfOp>(loc, global);
     Value cst0 = builder.create<LLVM::ConstantOp>(
         loc, IntegerType::get(builder.getContext(), 64),
         builder.getIntegerAttr(builder.getIndexType(), 0));
     return builder.create<LLVM::GEPOp>(

mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp

static Value getOrCreateGlobalString(Location loc, OpBuilder &builder,
     LLVM::GlobalOp global;
     if (!(global = module.lookupSymbol<LLVM::GlobalOp>(name))) {
       OpBuilder::InsertionGuard insertGuard(builder);
       builder.setInsertionPointToStart(module.getBody());
       auto type = LLVM::LLVMArrayType::get(
           IntegerType::get(builder.getContext(), 8), value.size());
       global = builder.create<LLVM::GlobalOp>(loc, type, /*isConstant=*/true,
                                               LLVM::Linkage::Internal, name,
-                                              builder.getStringAttr(value));
+                                              builder.getStringAttr(value),
+                                              /*alignment=*/0);
     }
 
     // Get the pointer to the first character in the global string.
     Value globalPtr = builder.create<LLVM::AddressOfOp>(loc, global);
     Value cst0 = builder.create<LLVM::ConstantOp>(
         loc, IntegerType::get(builder.getContext(), 64),
         builder.getIntegerAttr(builder.getIndexType(), 0));
     return builder.create<LLVM::GEPOp>(

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

 def LLVM_GlobalOp : LLVM_Op<"mlir.global",
     [IsolatedFromAbove, SingleBlockImplicitTerminator<"ReturnOp">, Symbol]> {
   let arguments = (ins
     TypeAttr:$type,
     UnitAttr:$constant,
     StrAttr:$sym_name,
     Linkage:$linkage,
     OptionalAttr<AnyAttr>:$value,
+    OptionalAttr<I64Attr>:$alignment,
     DefaultValuedAttr<Confined<I32Attr, [IntNonNegative]>, "0">:$addr_space,
     OptionalAttr<UnnamedAddr>:$unnamed_addr,
     OptionalAttr<StrAttr>:$section
   );
   let summary = "LLVM dialect global.";
   let description = [{
     Since MLIR allows for arbitrary operations to be present at the top level,
     global variables are defined using the `llvm.mlir.global` operation. Both
let description = [{
     ```
 
     Similarly to functions, globals have a linkage attribute. In the custom
     syntax, this attribute is placed between `llvm.mlir.global` and the optional
     `constant` keyword. If the attribute is omitted, `external` linkage is
     assumed by default.
 
     Examples:
 
     ```mlir
     // A constant with internal linkage will not participate in linking.
     llvm.mlir.global internal constant @cst(42 : i32) : i32
 
     // By default, "external" linkage is assumed and the global participates in
     // symbol resolution at link-time.
     llvm.mlir.global @glob(0 : f32) : f32
+
+    // Alignment is optional
+    llvm.mlir.global private constant @y(dense<1.0> : tensor<8xf32>) : !llvm.array<8 x f32>
+    ```
+
+    Like global variables in LLVM IR, globals can have an (optional)
+    alignment attribute using keyword `alignment`. The integer value of the
+    alignment must be a positive integer that is a power of 2.
+
+    Examples:
+
+    ```mlir
+    // Alignment is optional
+    llvm.mlir.global private constant @y(dense<1.0> : tensor<8xf32>) { alignment = 32 : i64 } : !llvm.array<8 x f32>
     ```
+
   }];
   let regions = (region AnyRegion:$initializer);
 
   let builders = [
     OpBuilder<(ins "Type":$type, "bool":$isConstant, "Linkage":$linkage,
-      "StringRef":$name, "Attribute":$value, CArg<"unsigned", "0">:$addrSpace,
+      "StringRef":$name, "Attribute":$value,
+      CArg<"uint64_t", "0">:$alignment,
+      CArg<"unsigned", "0">:$addrSpace,
       CArg<"ArrayRef<NamedAttribute>", "{}">:$attrs)>
   ];
 
   let extraClassDeclaration = [{
     /// Return the LLVM type of the global.
     Type getType() {
       return type();
     }
     /// Return the initializer attribute if it exists, or a null attribute.
     Attribute getValueOrNull() {
       return value().getValueOr(Attribute());
     }
     /// Return the initializer region. This may be empty, but if it is not it
     /// terminates in an `llvm.return` op with the initializer value.
     Region &getInitializerRegion() {
       return getOperation()->getRegion(0);
     }
     /// Return the initializer block. If the initializer region is empty this
     /// is nullptr. If it is not nullptr, it terminates with an `llvm.return`
     /// op with the initializer value.
     Block *getInitializerBlock() {

mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp

for (auto en : llvm::enumerate(gpuFuncOp.getWorkgroupAttributions())) {
     auto elementType =
         typeConverter->convertType(type.getElementType()).template cast<Type>();
     auto arrayType = LLVM::LLVMArrayType::get(elementType, numElements);
     std::string name = std::string(
         llvm::formatv("__wg_{0}_{1}", gpuFuncOp.getName(), en.index()));
     auto globalOp = rewriter.create<LLVM::GlobalOp>(
         gpuFuncOp.getLoc(), arrayType, /*isConstant=*/false,
         LLVM::Linkage::Internal, name, /*value=*/Attribute(),
-        gpu::GPUDialect::getWorkgroupAddressSpace());
+        /*alignment=*/0, gpu::GPUDialect::getWorkgroupAddressSpace());
     workgroupBuffers.push_back(globalOp);
   }
 
   // Rewrite the original GPU function to an LLVM function.
   auto funcType = typeConverter->convertType(gpuFuncOp.getType())
                       .template cast<LLVM::LLVMPointerType>()
                       .getElementType();
 

mlir/lib/Conversion/SPIRVToLLVM/ConvertLaunchFuncToLLVMCalls.cpp

for (auto operand : llvm::enumerate(kernelOperands)) {
           createGlobalVariableWithBindName(spirvGlobal, spvModuleName);
       // Check if this variable has already been created.
       auto dstGlobal = module.lookupSymbol<LLVM::GlobalOp>(name);
       if (!dstGlobal) {
         OpBuilder::InsertionGuard guard(rewriter);
         rewriter.setInsertionPointToStart(module.getBody());
         dstGlobal = rewriter.create<LLVM::GlobalOp>(
             loc, dstGlobalType,
-            /*isConstant=*/false, LLVM::Linkage::Linkonce, name, Attribute());
+            /*isConstant=*/false, LLVM::Linkage::Linkonce, name, Attribute(),
+            /*alignment=*/0);
         rewriter.setInsertionPoint(launchOp);
       }
 
       // Copy the data from src operand pointer to dst global variable. Save
       // src, dst and size so that we can copy data back after emulating the
       // kernel call.
       Value dst = rewriter.create<LLVM::AddressOfOp>(loc, dstGlobal);
       copy(loc, dst, src, sizeBytes, rewriter);

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp

if (!values.empty()) {
       auto arrayType = LLVM::LLVMArrayType::get(llvmI32Type, values.size());
       fields.push_back(arrayType);
     }
     auto structType = LLVM::LLVMStructType::getLiteral(context, fields);
 
     // Create `llvm.mlir.global` with initializer region containing one block.
     auto global = rewriter.create<LLVM::GlobalOp>(
         UnknownLoc::get(context), structType, /*isConstant=*/true,
-        LLVM::Linkage::External, executionModeInfoName, Attribute());
+        LLVM::Linkage::External, executionModeInfoName, Attribute(),
+        /*alignment=*/0);
     Location loc = global.getLoc();
     Region &region = global.getInitializerRegion();
     Block *block = rewriter.createBlock(&region);
 
     // Initialize the struct and set the execution mode value.
     rewriter.setInsertionPoint(block, block->begin());
     Value structValue = rewriter.create<LLVM::UndefOp>(loc, structType);
     IntegerAttr executionModeAttr = op.execution_modeAttr();
matchAndRewrite(spirv::GlobalVariableOp op, ArrayRef<Value> operands,
     // module and cannot be accessed by other modules. However, a module may be
     // written to export or import functions and global (module scope)
     // variables.". Therefore, map 'Private' storage class to private linkage,
     // 'Input' and 'Output' to external linkage.
     auto linkage = storageClass == spirv::StorageClass::Private
                        ? LLVM::Linkage::Private
                        : LLVM::Linkage::External;
     rewriter.replaceOpWithNewOp<LLVM::GlobalOp>(
-        op, dstType, isConstant, linkage, op.sym_name(), Attribute());
+        op, dstType, isConstant, linkage, op.sym_name(), Attribute(),
+        /*alignment=*/0);
     return success();
   }
 };
 
 /// Converts SPIR-V cast ops that do not have straightforward LLVM
 /// equivalent in LLVM dialect.
 template <typename SPIRVOp, typename LLVMExtOp, typename LLVMTruncOp>
 class IndirectCastPattern : public SPIRVToLLVMConversion<SPIRVOp> {

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

if (!global.isExternal() && !global.isUninitialized()) {
       // For scalar memrefs, the global variable created is of the element type,
       // so unpack the elements attribute to extract the value.
       if (type.getRank() == 0)
         initialValue = elementsAttr.getValue({});
     }
 
     rewriter.replaceOpWithNewOp<LLVM::GlobalOp>(
         global, arrayTy, global.constant(), linkage, global.sym_name(),
-        initialValue, type.getMemorySpaceAsInt());
+        initialValue, /*alignment=*/0, type.getMemorySpaceAsInt());
     return success();
   }
 };
 
 /// GetGlobalMemrefOp is lowered into a Memref descriptor with the pointer to
 /// the first element stashed into the descriptor. This reuses
 /// `AllocLikeOpLowering` to reuse the Memref descriptor construction.
 struct GetGlobalMemrefOpLowering : public AllocLikeOpLowering {

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/SourceMgr.h"
 
+#include <iostream>
+
 using namespace mlir;
 using namespace mlir::LLVM;
 
 static constexpr const char kVolatileAttrName[] = "volatile_";
 static constexpr const char kNonTemporalAttrName[] = "nontemporal";
 
 #include "mlir/Dialect/LLVMIR/LLVMOpsEnums.cpp.inc"
 #include "mlir/Dialect/LLVMIR/LLVMOpsInterfaces.cpp.inc"

 static StringRef getLinkageAttrName() { return "linkage"; }
 
 /// Returns the name used for the unnamed_addr attribute. This *must* correspond
 /// to the name of the attribute in ODS.
 static StringRef getUnnamedAddrAttrName() { return "unnamed_addr"; }
 
 void GlobalOp::build(OpBuilder &builder, OperationState &result, Type type,
                      bool isConstant, Linkage linkage, StringRef name,
-                     Attribute value, unsigned addrSpace,
+                     Attribute value, uint64_t alignment, unsigned addrSpace,
                      ArrayRef<NamedAttribute> attrs) {
   result.addAttribute(SymbolTable::getSymbolAttrName(),
                       builder.getStringAttr(name));
   result.addAttribute("type", TypeAttr::get(type));
   if (isConstant)
     result.addAttribute("constant", builder.getUnitAttr());
   if (value)
     result.addAttribute("value", value);
+
+  // Only add an alignment attribute if the "alignment" input
+  // is different from 0. The value must also be a power of two, but
+  // this is tested in GlobalOp::verify, not here.
+  if (alignment != 0)
+    result.addAttribute("alignment", builder.getI64IntegerAttr(alignment));
+
   result.addAttribute(getLinkageAttrName(),
                       builder.getI64IntegerAttr(static_cast<int64_t>(linkage)));
   if (addrSpace != 0)
     result.addAttribute("addr_space", builder.getI32IntegerAttr(addrSpace));
   result.attributes.append(attrs.begin(), attrs.end());
   result.addRegion();
 }
 
 static void printGlobalOp(OpAsmPrinter &p, GlobalOp op) {
   p << op.getOperationName() << ' ' << stringifyLinkage(op.linkage()) << ' ';
   if (op.unnamed_addr())
     p << stringifyUnnamedAddr(*op.unnamed_addr()) << ' ';
   if (op.constant())
     p << "constant ";
   p.printSymbolName(op.sym_name());
   p << '(';
   if (auto value = op.getValueOrNull())
     p.printAttribute(value);
   p << ')';
+  // Note that the alignment attribute is printed using the
+  // default syntax here, even though it is an inherent attribute
+  // (as defined in https://mlir.llvm.org/docs/LangRef/#attributes)
   p.printOptionalAttrDict(op->getAttrs(),
                           {SymbolTable::getSymbolAttrName(), "type", "constant",
                            "value", getLinkageAttrName(),
                            getUnnamedAddrAttrName()});
 
   // Print the trailing type unless it's a string global.
   if (op.getValueOrNull().dyn_cast_or_null<StringAttr>())
     return;
   p << " : " << op.type();
static int parseOptionalKeywordAlternative(OpAsmParser &parser,
   for (auto en : llvm::enumerate(keywords)) {
     if (succeeded(parser.parseOptionalKeyword(en.value())))
       return en.index();
   }
   return -1;
 }
 
 namespace {
-template <typename Ty> struct EnumTraits {};
+template <typename Ty>
+struct EnumTraits {};
 
 #define REGISTER_ENUM_TYPE(Ty)                                                 \
-  template <> struct EnumTraits<Ty> {                                          \
+  template <>                                                                  \
+  struct EnumTraits<Ty> {                                                      \
     static StringRef stringify(Ty value) { return stringify##Ty(value); }      \
     static unsigned getMaxEnumVal() { return getMaxEnumValFor##Ty(); }         \
   }
 
 REGISTER_ENUM_TYPE(Linkage);
 REGISTER_ENUM_TYPE(UnnamedAddr);
 } // end namespace
 
 template <typename EnumTy>
 static ParseResult parseOptionalLLVMKeyword(OpAsmParser &parser,
                                             OperationState &result,
                                             StringRef name) {
   SmallVector<StringRef, 10> names;
   for (unsigned i = 0, e = getMaxEnumValForLinkage(); i <= e; ++i)
     names.push_back(EnumTraits<EnumTy>::stringify(static_cast<EnumTy>(i)));
 
   int index = parseOptionalKeywordAlternative(parser, names);
   if (index == -1)
     return failure();
   result.addAttribute(name, parser.getBuilder().getI64IntegerAttr(index));
   return success();
 }
 
 // operation ::= `llvm.mlir.global` linkage? `constant`? `@` identifier
-//               `(` attribute? `)` attribute-list? (`:` type)? region?
+//               `(` attribute? `)` align? attribute-list? (`:` type)? region?
+// align     ::= `align` `=` UINT64
 //
 // The type can be omitted for string attributes, in which case it will be
 // inferred from the value of the string as [strlen(value) x i8].
 static ParseResult parseGlobalOp(OpAsmParser &parser, OperationState &result) {
   if (failed(parseOptionalLLVMKeyword<Linkage>(parser, result,
                                                getLinkageAttrName())))
     result.addAttribute(getLinkageAttrName(),
                         parser.getBuilder().getI64IntegerAttr(
if (parser.parseOptionalRParen()) {
     if (parser.parseAttribute(value, "value", result.attributes) ||
         parser.parseRParen())
       return failure();
   }
 
   SmallVector<Type, 1> types;
   if (parser.parseOptionalAttrDict(result.attributes) ||
       parser.parseOptionalColonTypeList(types))
     return failure();
 
   if (types.size() > 1)
     return parser.emitError(parser.getNameLoc(), "expected zero or one type");
 
   Region &initRegion = *result.addRegion();
   if (types.empty()) {
     if (auto strAttr = value.dyn_cast_or_null<StringAttr>()) {
       MLIRContext *context = parser.getBuilder().getContext();
       auto arrayType = LLVM::LLVMArrayType::get(IntegerType::get(context, 8),
                                                 strAttr.getValue().size());
static LogicalResult verify(GlobalOp op) {
   if (op.linkage() == Linkage::Appending) {
     if (!op.getType().isa<LLVMArrayType>()) {
       return op.emitOpError()
              << "expected array type for '"
              << stringifyLinkage(Linkage::Appending) << "' linkage";
     }
   }
 
+  Optional<uint64_t> alignAttr = op.alignment();
+  if (alignAttr.hasValue()) {
+    uint64_t value = alignAttr.getValue();
+    if (!llvm::isPowerOf2_64(value))
+      return op->emitError() << "alignment attribute is not a power of 2";
+  }
+
   return success();
 }
 
 //===----------------------------------------------------------------------===//
 // Printing/parsing for LLVM::ShuffleVectorOp.
 //===----------------------------------------------------------------------===//
 // Expects vector to be of wrapped LLVM vector type and position to be of
 // wrapped LLVM i32 type.
Value mlir::LLVM::createGlobalString(Location loc, OpBuilder &builder,
   assert(module && "builder points to an op outside of a module");
 
   // Create the global at the entry of the module.
   OpBuilder moduleBuilder(module.getBodyRegion(), builder.getListener());
   MLIRContext *ctx = builder.getContext();
   auto type = LLVM::LLVMArrayType::get(IntegerType::get(ctx, 8), value.size());
   auto global = moduleBuilder.create<LLVM::GlobalOp>(
       loc, type, /*isConstant=*/true, linkage, name,
-      builder.getStringAttr(value));
+      builder.getStringAttr(value), /*alignment=*/0);
 
   // Get the pointer to the first character in the global string.
   Value globalPtr = builder.create<LLVM::AddressOfOp>(loc, global);
   Value cst0 = builder.create<LLVM::ConstantOp>(
       loc, IntegerType::get(ctx, 64),
       builder.getIntegerAttr(builder.getIndexType(), 0));
   return builder.create<LLVM::GEPOp>(
       loc, LLVM::LLVMPointerType::get(IntegerType::get(ctx, 8)), globalPtr,

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

GlobalOp Importer::processGlobal(llvm::GlobalVariable *GV) {
 
   OpBuilder b(module.getBody(), getGlobalInsertPt());
   Attribute valueAttr;
   if (GV->hasInitializer())
     valueAttr = getConstantAsAttr(GV->getInitializer());
   Type type = processType(GV->getValueType());
   if (!type)
     return nullptr;
-  GlobalOp op = b.create<GlobalOp>(
-      UnknownLoc::get(context), type, GV->isConstant(),
-      convertLinkageFromLLVM(GV->getLinkage()), GV->getName(), valueAttr);
+
+  uint64_t alignment = 0;
+  llvm::MaybeAlign maybeAlign = GV->getAlign();
+  if (maybeAlign.hasValue()) {
+    llvm::Align align = maybeAlign.getValue();
+    alignment = align.value();
+  }
+
+  GlobalOp op =
+      b.create<GlobalOp>(UnknownLoc::get(context), type, GV->isConstant(),
+                         convertLinkageFromLLVM(GV->getLinkage()),
+                         GV->getName(), valueAttr, alignment);
+
   if (GV->hasInitializer() && !valueAttr) {
     Region &r = op.getInitializerRegion();
     currentEntryBlock = b.createBlock(&r);
     b.setInsertionPoint(currentEntryBlock, currentEntryBlock->begin());
     Value v = processConstant(GV->getInitializer());
     if (!v)
       return nullptr;
     b.create<ReturnOp>(op.getLoc(), ArrayRef<Value>({v}));
   }
   if (GV->hasAtLeastLocalUnnamedAddr())
     op.unnamed_addrAttr(UnnamedAddrAttr::get(
         context, convertUnnamedAddrFromLLVM(GV->getUnnamedAddr())));
   if (GV->hasSection())
     op.sectionAttr(b.getStringAttr(GV->getSection()));
+
   return globals[GV] = op;
 }
 
 Value Importer::processConstant(llvm::Constant *c) {
   OpBuilder bEntry(currentEntryBlock, currentEntryBlock->begin());
   if (Attribute attr = getConstantAsAttr(c)) {
     // These constants can be represented as attributes.
     OpBuilder b(currentEntryBlock, currentEntryBlock->begin());

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

for (auto op : getModuleBody(mlirModule).getOps<LLVM::GlobalOp>()) {
     auto linkage = convertLinkageToLLVM(op.linkage());
     auto addrSpace = op.addr_space();
     auto *var = new llvm::GlobalVariable(
         *llvmModule, type, op.constant(), linkage,
         shouldDropGlobalInitializer(linkage, cst) ? nullptr : cst,
         op.sym_name(),
         /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal, addrSpace);
 
     if (op.unnamed_addr().hasValue())
       var->setUnnamedAddr(convertUnnamedAddrToLLVM(*op.unnamed_addr()));
 
     if (op.section().hasValue())
       var->setSection(*op.section());
 
+    Optional<uint64_t> alignment = op.alignment();
+    if (alignment.hasValue())
+      var->setAlignment(llvm::MaybeAlign(alignment.getValue()));
+
     globalsMapping.try_emplace(op, var);
   }
 
   // Convert global variable bodies. This is done after all global variables
   // have been created in LLVM IR because a global body may refer to another
   // global or itself. So all global variables need to be mapped first.
   for (auto op : getModuleBody(mlirModule).getOps<LLVM::GlobalOp>()) {
     if (Block *initializer = op.getInitializerBlock()) {

mlir/test/Dialect/LLVMIR/global.mlir

 // RUN: mlir-opt -split-input-file -verify-diagnostics %s | FileCheck %s
 
 // CHECK: llvm.mlir.global external @default_external
 llvm.mlir.global @default_external() : i64
 
 // CHECK: llvm.mlir.global external constant @default_external_constant
 llvm.mlir.global constant @default_external_constant(42) : i64
 
 // CHECK: llvm.mlir.global internal @global(42 : i64) : i64
 llvm.mlir.global internal @global(42 : i64) : i64
 
+// CHECK: llvm.mlir.global private @aligned_global(42 : i64) {aligned = 64 : i64} : i64
+llvm.mlir.global private @aligned_global(42 : i64) {aligned = 64} : i64
+
+// CHECK: llvm.mlir.global private constant @aligned_global_const(42 : i64) {aligned = 32 : i64} : i64
+llvm.mlir.global private constant @aligned_global_const(42 : i64) {aligned = 32} : i64
+
 // CHECK: llvm.mlir.global internal constant @constant(3.700000e+01 : f64) : f32
 llvm.mlir.global internal constant @constant(37.0) : f32
 
 // CHECK: llvm.mlir.global internal constant @".string"("foobar")
 llvm.mlir.global internal constant @".string"("foobar") : !llvm.array<6 x i8>
 
 // CHECK: llvm.mlir.global internal @string_notype("1234567")
 llvm.mlir.global internal @string_notype("1234567")

mlir/test/Dialect/LLVMIR/invalid.mlir

 // RUN: mlir-opt -allow-unregistered-dialect %s -split-input-file -verify-diagnostics
 
+// expected-error@+1{{alignment attribute is not a power of 2}}
+llvm.mlir.global private @invalid_global_alignment(42 : i64) {alignment = 63} : i64
+
+// -----
+
 // expected-error@+1{{expected llvm.noalias argument attribute to be a unit attribute}}
 func @invalid_noalias(%arg0: i32 {llvm.noalias = 3}) {
   "llvm.return"() : () -> ()
 }
 
 // -----
 
 // expected-error@+1{{llvm.align argument attribute of non integer type}}

mlir/test/Target/LLVMIR/import.ll

 ; RUN: mlir-translate -import-llvm %s | FileCheck %s
 
 %struct.t = type {}
 %struct.s = type { %struct.t, i64 }
 
-; CHECK: llvm.mlir.global external @g1() : !llvm.struct<"struct.s", (struct<"struct.t", ()>, i64)>
+; CHECK: llvm.mlir.global external @g1() {alignment = 8 : i64} : !llvm.struct<"struct.s", (struct<"struct.t", ()>, i64)>
 @g1 = external global %struct.s, align 8
-; CHECK: llvm.mlir.global external @g2() : f64
+; CHECK: llvm.mlir.global external @g2() {alignment = 8 : i64} : f64
 @g2 = external global double, align 8
 ; CHECK: llvm.mlir.global internal @g3("string")
 @g3 = internal global [6 x i8] c"string"
 
 ; CHECK: llvm.mlir.global external @g5() : vector<8xi32>
 @g5 = external global <8 x i32>
 
+; CHECK: llvm.mlir.global private @alig32(42 : i64) {alignment = 32 : i64} : i64
+@alig32 = private global i64 42, align 32
+
+; CHECK: llvm.mlir.global private @alig64(42 : i64) {alignment = 64 : i64} : i64
+@alig64 = private global i64 42, align 64
+
 @g4 = external global i32, align 8
 ; CHECK: llvm.mlir.global internal constant @int_gep() : !llvm.ptr<i32> {
 ; CHECK-DAG:   %[[addr:[0-9]+]] = llvm.mlir.addressof @g4 : !llvm.ptr<i32>
 ; CHECK-DAG:   %[[c2:[0-9]+]] = llvm.mlir.constant(2 : i32) : i32
 ; CHECK-NEXT:  %[[gepinit:[0-9]+]] = llvm.getelementptr %[[addr]][%[[c2]]] : (!llvm.ptr<i32>, i32) -> !llvm.ptr<i32>
 ; CHECK-NEXT:  llvm.return %[[gepinit]] : !llvm.ptr<i32>
 ; CHECK-NEXT: }
 @int_gep = internal constant i32* getelementptr (i32, i32* @g4, i32 2)

mlir/test/Target/LLVMIR/llvmir.mlir

 // RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
 
+// CHECK: @global_aligned32 = private global i64 42, align 32
+"llvm.mlir.global"() ({}) {sym_name = "global_aligned32", type = i64, value = 42 : i64, linkage = 0, alignment = 32} : () -> ()
+
+// CHECK: @global_aligned64 = private global i64 42, align 64
+llvm.mlir.global private @global_aligned64(42 : i64) {alignment = 64 : i64} : i64
+
+// CHECK: @global_aligned64_native = private global i64 42, align 64
+llvm.mlir.global private @global_aligned64_native(42 : i64) { alignment = 64 } : i64
+
 // CHECK: @i32_global = internal global i32 42
 llvm.mlir.global internal @i32_global(42: i32) : i32
 
 // CHECK: @i32_const = internal constant i53 52
 llvm.mlir.global internal constant @i32_const(52: i53) : i53
 
 // CHECK: @int_global_array = internal global [3 x i32] [i32 62, i32 62, i32 62]
 llvm.mlir.global internal @int_global_array(dense<62> : vector<3xi32>) : !llvm.array<3 x i32>

 // CHECK: ![[GROUP_NODE1]] = distinct !{}
 // CHECK: ![[GROUP_NODE2]] = distinct !{}
 // CHECK: ![[UNROLL_DISABLE_NODE]] = !{!"llvm.loop.unroll.disable", i1 true}
 // CHECK: ![[LICM_DISABLE_NODE]] = !{!"llvm.licm.disable", i1 true}
 // CHECK: ![[INTERLEAVE_NODE]] = !{!"llvm.loop.interleave.count", i32 1}
 // CHECK: ![[PIPELINE_DISABLE_NODE]] = !{!"llvm.loop.pipeline.disable", i1 true}
 // CHECK: ![[II_NODE]] = !{!"llvm.loop.pipeline.initiationinterval", i32 2}
 // CHECK: ![[ACCESS_GROUPS_NODE]] = !{![[GROUP_NODE1]], ![[GROUP_NODE2]]}
+