[MLIR][MemRef] Change builders with int alignment params to llvm::MaybeAlign

[jiang1997]

Change remaining OpBuilder methods to use llvm::MaybeAlign instead of uint64_t for alignment parameters.

[llvmbot]

@llvm/pr-subscribers-mlir-vector
@llvm/pr-subscribers-mlir-memref

@llvm/pr-subscribers-mlir

Author: None (jiang1997)

Changes

This completes the alignment type migration missing from PR #158137 related to issue #155677.
Update remaining OpBuilder methods to use llvm::MaybeAlign instead of uint64_t for alignment parameters.

---

Full diff: https://github.com/llvm/llvm-project/pull/159449.diff

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td (+12-12)

diff --git a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
index 671cc05e963b4..bdc505d765b14 100644
--- a/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
+++ b/mlir/include/mlir/Dialect/MemRef/IR/MemRefOps.td
@@ -1237,28 +1237,28 @@ def LoadOp : MemRef_Op<"load",
     OpBuilder<(ins "Value":$memref,
                    "ValueRange":$indices,
                    CArg<"bool", "false">:$nontemporal,
-                   CArg<"uint64_t", "0">:$alignment), [{
+                   CArg<"llvm::MaybeAlign", "llvm::MaybeAlign()">:$alignment), [{
       return build($_builder, $_state, memref, indices, nontemporal,
-                   alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
-                                    nullptr);
+                   alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
+                               nullptr);
     }]>,
     OpBuilder<(ins "Type":$resultType,
                    "Value":$memref,
                    "ValueRange":$indices,
                    CArg<"bool", "false">:$nontemporal,
-                   CArg<"uint64_t", "0">:$alignment), [{
+                   CArg<"llvm::MaybeAlign", "llvm::MaybeAlign()">:$alignment), [{
       return build($_builder, $_state, resultType, memref, indices, nontemporal,
-                   alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
-                                    nullptr);
+                   alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
+                               nullptr);
     }]>,
     OpBuilder<(ins "TypeRange":$resultTypes,
                    "Value":$memref,
                    "ValueRange":$indices,
                    CArg<"bool", "false">:$nontemporal,
-                   CArg<"uint64_t", "0">:$alignment), [{
+                   CArg<"llvm::MaybeAlign", "llvm::MaybeAlign()">:$alignment), [{
       return build($_builder, $_state, resultTypes, memref, indices, nontemporal,
-                   alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
-                                    nullptr);
+                   alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
+                               nullptr);
     }]>
   ];
 
@@ -1971,10 +1971,10 @@ def MemRef_StoreOp : MemRef_Op<"store",
                    "Value":$memref,
                    "ValueRange":$indices,
                    CArg<"bool", "false">:$nontemporal,
-                   CArg<"uint64_t", "0">:$alignment), [{
+                   CArg<"llvm::MaybeAlign", "llvm::MaybeAlign()">:$alignment), [{
       return build($_builder, $_state, valueToStore, memref, indices, nontemporal,
-                   alignment != 0 ? $_builder.getI64IntegerAttr(alignment) :
-                                    nullptr);
+                   alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
+                               nullptr);
     }]>,
     OpBuilder<(ins "Value":$valueToStore, "Value":$memref), [{
       $_state.addOperands(valueToStore);

[jiang1997]

@amd-eochoalo
review request

[amd-eochoalo]

Hello @jiang1997 thanks again for your contribution. This looks exactly like what the ticket described, but give me a couple of days to review the overall design (also what will come after this PR) just to make sure that these two changes together make sense. Thanks for your patience!

[banach-space]

Wouldn't this be sufficient?

Suggested change

	alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
	alignment ? $_builder.getI64IntegerAttr(alignment.value()) :

[amd-eochoalo]

One can't do that but can do:

Suggested change

	alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
	alignment ? $_builder.getI64IntegerAttr((*alignment).value()) :

or also

Suggested change

	alignment ? $_builder.getI64IntegerAttr(alignment.valueOrOne().value()) :
	alignment ? $_builder.getI64IntegerAttr(alignment->value()) :

because MaybeAlign inherits from std::optional and calling value will return an llvm::Align

I think the second one here is better.

[amd-eochoalo]

Hi @jiang1997 , thanks for your patience.

Can you clean up the constructors that use valueOrOne().value() to just use alignment->value()? After that I think it would be good to merge.

I think this is a good change but it is not the final one. There is one after this one which I wanted to verify how it would look like after this change was applied. The next change would be to add an OpInterface that defines a default method (possibly called) getMaybeAlign which would return the alignment as an llvm::MaybeAlign. With this, we could now use it with the constructors that receive an llvm::MaybeAlign. E.g.,

          [&](OpBuilder &builder, Location loc) {
            auto loadedValue = memref::LoadOp::create(
                builder, loc, base, indices, /*nontemporal=*/false,
                maskedLoad.getMaybeAlign()):

Let me know if you want to work on that one too, but no pressure. I think this last one may receive more reviewer comments.

[jiang1997]

Hi @jiang1997 , thanks for your patience.

Can you clean up the constructors that use valueOrOne().value() to just use alignment->value()? After that I think it would be good to merge.

I think this is a good change but it is not the final one. There is one after this one which I wanted to verify how it would look like after this change was applied. The next change would be to add an OpInterface that defines a default method (possibly called) getMaybeAlign which would return the alignment as an llvm::MaybeAlign. With this, we could now use it with the constructors that receive an llvm::MaybeAlign. E.g.,

          [&](OpBuilder &builder, Location loc) {
            auto loadedValue = memref::LoadOp::create(
                builder, loc, base, indices, /*nontemporal=*/false,
                maskedLoad.getMaybeAlign()):

Let me know if you want to work on that one too, but no pressure. I think this last one may receive more reviewer comments.

Yes, I know this isn’t the final one— the original issue also mentioned adding the new interface. I’m willing to keep contributing.

[amd-eochoalo]

Thank you! @jiang1997