[mlir][test] Fix IR/AttributeTest.cpp compilation on Solaris

[ro]

Summary

The IR/AttributeTest.cpp test fails to compile on Solaris:

/vol/llvm/src/llvm-project/local/mlir/unittests/IR/AttributeTest.cpp:223:36: error: no matching function for call to 'allocate'
      AttrT::get(type, "resource", UnmanagedAsmResourceBlob::allocate(data));
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/vol/llvm/src/llvm-project/local/mlir/unittests/IR/AttributeTest.cpp:237:3: note: in instantiation of function template specialization 'checkNativeAccess<mlir::detail::DenseResourceElementsAttrBase<int8_t>, char>' requested here
  checkNativeAccess<AttrT, T>(builder.getContext(), llvm::makeArrayRef(data),
  ^
/vol/llvm/src/llvm-project/local/mlir/unittests/IR/AttributeTest.cpp:258:3: note: in instantiation of function template specialization 'checkNativeIntAccess<mlir::detail::DenseResourceElementsAttrBase<int8_t>, char>' requested here
  checkNativeIntAccess<DenseI8ResourceElementsAttr, int8_t>(builder, 8);
  ^
/vol/llvm/src/llvm-project/local/mlir/include/mlir/IR/AsmState.h:221:3: note: candidate template ignored: requirement '!std::is_same<char, char>::value' was not satisfied [with T = char]
  allocate(ArrayRef<T> data, bool dataIsMutable = false) {
  ^
/vol/llvm/src/llvm-project/local/mlir/include/mlir/IR/AsmState.h:214:26: note: candidate function not viable: requires at least 2 arguments, but 1 was provided
  static AsmResourceBlob allocate(ArrayRef<char> data, size_t align,
                         ^

Because char is signed by default on Solaris and int8_t is char. std::is_same<int8_t, char> is true unlike elsewhere, rejecting the one-arg allocate overload.

Fixed by renaming the two overloads to avoid the ambiguity.

Tested on amd64-pc-solaris2.11 ,sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

[jpienaar]

Thanks!

[ro]

Turns out this patch broke the Windows buildbot Windows buildbot.

C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\unittests\IR\AttributeTest.cpp(223): error C2220: the following warning is treated as an error
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\unittests\IR\AttributeTest.cpp(266): note: see reference to function template instantiation 'void checkNativeAccess<mlir::DenseF32ResourceElementsAttr,T>(mlir::MLIRContext *,llvm::ArrayRef<T>,mlir::Type)' being compiled
        with
        [
            T=float
        ]
C:\buildbot\mlir-x64-windows-ninja\llvm-project\mlir\unittests\IR\AttributeTest.cpp(223): warning C4305: 'argument': truncation from 'size_t' to 'bool

However, I don't have the slightest idea what's wrong here or what to do.

[mehdi_amini]

There are two signatures:

static AsmResourceBlob allocate(ArrayRef<char> data, size_t align,
                                  bool dataIsMutable = false) {

and:

template <typename T>
static std::enable_if_t<!std::is_same<T, char>::value, AsmResourceBlob>
allocate(ArrayRef<T> data, bool dataIsMutable = false) {
  return allocate(
      ArrayRef<char>((const char *)data.data(), data.size() * sizeof(T)),
      alignof(T));
}

For some reason Windows resolve to the second one?

[mehdi_amini]

Actually that makes sense, this patch does not seem right.

[ro]

In D131148#3700505, @mehdi_amini wrote:

Actually that makes sense, this patch does not seem right.

Sorry for the mess (and my lack of understanding of C++). I think I know now what's going on: as I mentioned, on Solaris char is signed and int8_t is char, while e.g. Linux uses signed char for int8_t. So std::is_same<int8_t, char> is true on Solaris, but false elsewhere.

To avoid this, I tried using std::make_signed_t<T> instead of plain T, however that fails for T = bool (make_signed_t needs an integral type), and so far I've not been able to get the syntax right to combine all this. It should be something like

is_integral<T> && !is_same<T, bool> ? std::make_signed_t<T> : T

I believe, unless there's a different route to address this issue.

[mehdi_amini]

I don't like the way the two overloads are set-up (static AsmResourceBlob allocate), before "fixing" it with more complicate template SFINAE we should look if we can revisit this a bit more from first principles: I'll summon @rriddle for this :)

[rriddle]

In D131148#3731766, @mehdi_amini wrote:

I don't like the way the two overloads are set-up (static AsmResourceBlob allocate), before "fixing" it with more complicate template SFINAE we should look if we can revisit this a bit more from first principles: I'll summon @rriddle for this :)

If we have problematic overloads (for better or worse), we should rename the overloads to be more clear what the intent is. i.e. for the align variant that could be allocateWithAlign, for the other it could be allocateInferAlign, etc..

[ro]

In D131148#3732341, @rriddle wrote:

If we have problematic overloads (for better or worse), we should rename the overloads to be more clear what the intent is. i.e. for the align variant that could be allocateWithAlign, for the other it could be allocateInferAlign, etc..

That works seamlessly. To keep the patch minimal, I've only changed class UnmanagedAsmResourceBlob. The same could be done to class HeapAsmResourceBlob for symmetry's sake.

[ro]

Rename UnmanagedAsmResourceBlob::allocate overloads.

Retested on amd64-pc-solaris2.11, sparcv9-sun-solaris2.11, and x86_64-pc-linux-gnu.

[mehdi_amini]

LG

mlir/unittests/IR/AttributeTest.cpp

 //===----------------------------------------------------------------------===//
 // DenseResourceElementsAttr
 //===----------------------------------------------------------------------===//
 
 template <typename AttrT, typename T>
 static void checkNativeAccess(MLIRContext *ctx, ArrayRef<T> data,
                               Type elementType) {
   auto type = RankedTensorType::get(data.size(), elementType);
-  auto attr =
-      AttrT::get(type, "resource", UnmanagedAsmResourceBlob::allocate(data));
+  auto attr = AttrT::get(type, "resource",
+                         UnmanagedAsmResourceBlob::allocate(data, alignof(T)));
 
   // Check that we can access and iterate the data properly.
   Optional<ArrayRef<T>> attrData = attr.tryGetAsArrayRef();
   EXPECT_TRUE(attrData.hasValue());
   EXPECT_EQ(*attrData, data);
 
   // Check that we cast to this attribute when possible.
   Attribute genericAttr = attr;