@rriddle Thanks for reviewing many times! I don't have commit access. Could you commit this patch if you don't have any other comments?

Hi, @rriddle, do you have any comments on this patch?

In D88719#2381285, @bondhugula wrote:

Looks good. Please address the remaining minor comments.

Rebased and reflected comments.

In D80272#2090239, @rriddle wrote:

Can you try Eli's suggestion in this patch?

Updated comments and inserted assert.

Use ulittle for eidian conversion.

Reflected the comment.

@rriddle Thanks for your review. I don't have commit access. If you don't have other comments, could you commit this patch?

Avoid warning message.

Avoid warning in assert().

Rebased.

Reflect comments.

@rriddle Thanks for your comments. I answered some of the comments.

Reflected comments and rebased.

Removed unnecessary headers.

Moved the convEndianBE into DenseIntOrFPElementsAttr class and removed EndianUtilities.

@mehdi_amini Thanks for your review! I reflected all of your comments.

Reflected @mehdi_amini's comments.

Implemented the conversion from LE to BE in AttributeParser and from BE to LE in AsmPrinter
, assuming that HEX in dense.attr is always LE.

@bondhugula @avarmapml Thanks for your comments! I updated the code to reflect your comments. I moved part of code in the lambda to Util.cpp. Could you review again?

Rebased and fixed a comment.

Updated test case to include two operation results.

Reflected comments and rebased.

@bondhugula @avarmapml I found the results(returend memref) of normalizable operation were not normalized by --normalize-memrefs.
In test case, %1 was not normalized. This patch fixes it.
%1 = "test.op_norm_ret"(%arg0, %arg1) : (memref<1x32768xf32>, memref<1x16x14x14xf32, #map1>) -> (memref<1x16x14x14xf32, #map1>)

Fix to remove warning message (newOp -> *newOp)

Rebased

@bondhugula It seems there is no additional comments. Could you commit also this patch?

@AlexEichenberger

glad to see that the traits make such changes easy...

Yes. The traits are very useful. We might need to add them in other ops depending on use-case, but it is easy.

Reflected the comment about map, and rebased

When we use load and store ops like the test in this patch, the --normalize-memrefs does not work. This patch fix it by adding MemRefsNormalizable trait in load and store ops.
If we can use affine.load and affine.store in this test, the normalization works, but sometimes affine.load can not be used. In the case, we need to use std.load.

@bondhugula @AlexEichenberger @avarmapml Do you have any other comments?

Updated comments and rename test func

Fixed comments and format

Rebased

Sorry, I may did wrong operaton... I will fix this.

Rebased and rephrased the comment.

@bondhugula @avarmapml Thanks for your comments! I added test and comments, and renamed the variables to reflect @bondhugula 's comments.

Added test and comments, and renamee variables to reflrect comments

This bug was discussed here https://llvm.discourse.group/t/memrefs-and-maps-for-tiling/1279/21
Following example failed because spv.EntryPoint "GLCompute" @empty can't be casted to CallOp. I created the example to reproduce errors happened in my code which uses EntryPoint op of my own dialect. This example may not be practical, but this reproduced the same error with my code.

In D80695#2266442, @mehdi_amini wrote:

In D80695#2265193, @imaihal wrote:

I thought native byte order should be written in dense.attr.

Well that's my point all along: let's not do that and make the serialization/deserialization cross-platform.

In D80695#2262569, @mehdi_amini wrote:

I don't understand what you're referring to right now: can you elaborate how a user would write the hex in BE?
I expect a users to form the data in memory in BE on a BE machine, and then the printer will print in LE form always.

In D80695#2262458, @mehdi_amini wrote:

If we can assume the hex data is always little-endian, it can be converted here, but we can't assume that.

Why? We control the printer and parser, don't we?

Thanks. I checked the source code. The dense attribute with hex data is parsed here https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/AttributeParser.cpp#L682-L684
If we can assume the hex data is always little-endian, it can be converted here, but we can't assume that. So, my understanding is that the hex data of dense-elements-hex.mlir should be generated depending on the endianness. Is this possible? I'm looking for similar examples.

In D80695#2251361, @mehdi_amini wrote:

Could the HEX dump be made independent of the platform? Having the printed format dumped on a BE loadable on a LE and vice-versa would be nicer I think.

Hi, I'm looking for reviewers for this patch. This patch is old. So, I need to update, but I would like to hear your comments about how we should pass this test on big-endian machines. Since current test includes HEX of little endian, this test fails on big-endian machines.
@jpienaar, @ftynse, @mehdi_amini, @rriddle @Smit You were reviewers for patches including this test before. Could you give me your comments?

@bondhugula Thanks for your review! I reflected your comments.
I think I don't have right to land this patch. Could you land this if you don't have additional comments?

Rebase

Rebase again

Rebase

Change CHECK to CHECK-NEXT

Thanks for your comments! I updated.

Remove redundant code

@rriddle Hi, is it possible to give me your comments about this patch? I'm asking because you were a reviewer of the previous patch including this file.

In D80272#2077654, @rriddle wrote:

In D80272#2077637, @efriedma wrote:

In general, I'd like to strongly discourage directly checking endian::system_endianness(), in favor of using the endian-sensitive read/write helpers from llvm/Support/Endian.h. For example, you can copy_n from an endian::ulittle64_t* to a uint64_t*. Using a distinct codepath for big-endian means more code, and that code gets less testing.

Thanks for the comments Eli! Didn't know about those. Will recommend the same in the future.

Reflect reviewer's comments.

Could anyone be a reviewer of this patch? This patch is about one of the MLIR test (dense-elements-hex.mlir).

Rebased with master

Fixed "No newline at end of file"

Fixed variable name to meet camelBack style

@ftynse and @mehdi_amini, Could you help to commit this if this patch ready for commit? I don't have write access to the repository.

Changed getHostCPUName() part to reflect the comment.

My wrong operation(submission) may add LLDB tag on this patch.

Thanks for the review! I changed the variable name to camelBack

• [NFC] Replace MaybeAlign with Align in TargetTransformInfo.
• [mlir][SystemZ] Fix incompatible datalayout in SystemZ

[mlir][SystemZ] Fix incompatible datalayout in SystemZ

Without this patch, following tests fail in SystemZ(z14)

MLIR :: mlir-cpu-runner/linalg_integration_test.mlir
MLIR :: mlir-cpu-runner/sgemm_naive_codegen.mlir
MLIR :: mlir-cpu-runner/simple.mlir
MLIR :: mlir-cpu-runner/unranked_memref.mlir
MLIR :: mlir-cpu-runner/utils.mlir

Error message is as follows.

Failure value returned from cantFail wrapped call
Added modules have incompatible data layouts: E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64 (module) vs E-m:e-i1:8:16-i8:8:16-i64:6\
4-f128:64-v128:64-a:8:16-n32:64 (jit)

@rriddle Is this patch ready for commit? If so, could you help to commit this? I think I don't have write access to the repository.