[Mlir-commits] [mlir] [WIP][MLIR] Use IntValidAlignment for alignment attributes (PR #158137)

[Erick Ochoa Lopez]

https://github.com/amd-eochoalo requested changes to this pull request.

Hello @jiang1997, thank you for contributing!

I will do a more thorough review a little bit later today (although it already looks quite good). I've noticed that the CI has detected some errors. You can also run locally and these errors should be detected locally as well. (Let me know if you need help running tests locally).

At least the first error reported in the CI comes from the file located in the following path `llvm-project/llvm-project/mlir/test/Dialect/MemRef/invalid.mlir`

Here is the full error log:

```
  # .---command stderr------------
  # | /home/gha/actions-runner/_work/llvm-project/llvm-project/mlir/test/Dialect/MemRef/invalid.mlir:384:58: error: unexpected error: custom op 'memref.global' 'memref.global' op attribute 'alignment' failed to satisfy constraint: 64-bit signless integer attribute whose value is positive and whose value is a power of two > 0
  # | memref.global "private" @gv : memref<4xf32> = dense<1.0> { alignment = 63 }
  # |                                                          ^
  # | /home/gha/actions-runner/_work/llvm-project/llvm-project/mlir/test/Dialect/MemRef/invalid.mlir:383:4: error: expected error "alignment attribute value 63 is not a power of 2" was not produced
  # | // expected-error @+1 {{alignment attribute value 63 is not a power of 2}}
  # |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  # `-----------------------------
  # error: command failed with exit status: 1
```

At least for this case (and possibly others) the `memref.global` operation has a custom verification. This custom verification can be found on file `mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp`. The test case above was expecting this custom verification's error message, but once you change the alignment property to be constrained by the new attribute, the verification message has changed.

What you can do in this case is change the test case to match the new error message and remove the couple lines of code that correspond to the verification of the alignment attribute inside `LogicalResult GlobalOp::verify`. Specifically, these ones:

```cxx
  if (std::optional<uint64_t> alignAttr = getAlignment()) {
    uint64_t alignment = *alignAttr;

    if (!llvm::isPowerOf2_64(alignment))
      return emitError() << "alignment attribute value " << alignment
                         << " is not a power of 2";
  }
```

As I mentioned earlier, I'll try to do a more thorough review later today, this is just of the first error here. Thanks for contributing!

https://github.com/llvm/llvm-project/pull/158137