This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Affine] Fix assumption on int type in memref elt size method
ClosedPublic

Authored by bondhugula on Mar 20 2023, 8:07 PM.

Details

Summary

Fix assumption on memref element type being int/float in memref elt size
related method and affine data copy generate.

Fixes https://github.com/llvm/llvm-project/issues/61310

Diff Detail

Event Timeline

bondhugula created this revision.Mar 20 2023, 8:07 PM
bondhugula requested review of this revision.Mar 20 2023, 8:07 PM
bondhugula added inline comments.Mar 20 2023, 8:08 PM
mlir/include/mlir/Dialect/Affine/Analysis/Utils.h
382

@ftynse I'm not sure this should be exposed from here or if there's something else I should use or if this should be renamed to something specific.

bondhugula added inline comments.Mar 20 2023, 10:01 PM
mlir/include/mlir/Dialect/Affine/Analysis/Utils.h
382

For index-typed memrefs and for memrefs in general, at this stage, we don't have layout information. So this is really returning a tentative or a "best-effort" size, which is what the clients of this method (affine passes) desire. This is being exposed from MLIR Affine Analysis library since it has at least two users among affine passes/utilities. In general, the clients are expected to be aware that this size may not be the final size of the buffer which may be layout/backend dependent.

ftynse added inline comments.Mar 21 2023, 6:07 AM
mlir/include/mlir/Dialect/Affine/Analysis/Utils.h
382

If this is called from the pass, it may be possible to obtain the DataLayoutAnalysis in the pass from the analysis manager, and query it for the DataLayout object of the module. Given that, this should be able to compute the element size the same way the allocation lowering does.

Otherwise, I'd call it getMemRefIntOrFloatEltSizeInBytes, or something similar that indicates it being only suitable for ints, floats and vectors thereof.

bondhugula marked an inline comment as done.

Incorporate review suggestion.

bondhugula added inline comments.Mar 21 2023, 9:09 PM
mlir/include/mlir/Dialect/Affine/Analysis/Utils.h
382

Thanks. Renamed to getMemRefIntOrFloatEltSizeInBytes. This is used from a free-standing utility in Affine LoopUtils although it's eventually used from passes in the upstream repo. It's expected to be used from other utilities downstream.

Lewuathe added inline comments.Mar 21 2023, 9:51 PM
mlir/lib/Dialect/Affine/Analysis/Utils.cpp
606

I'm not sure whether this approach is correct. But we assume the index type bit-width is set by kInternalStorageBitWidth, which is already used in the several parts.

https://reviews.llvm.org/D146019

Can we simply use this value for calculating the memref size as well?

bondhugula marked an inline comment as done.Mar 22 2023, 2:45 AM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/Analysis/Utils.cpp
606

Note that isIntOrFloat returns false for index types. So, we are returning nullopt (i.e., failing this method) when an provide an index typed memref is provided. It would be wrong to use a specific value since the width of index depends on the lowering target - it can be 32 bits, 64 bits or anything else.

ftynse accepted this revision.Mar 22 2023, 3:24 AM
ftynse added inline comments.
mlir/lib/Dialect/Affine/Analysis/Utils.cpp
606

I'm not sure whether this approach is correct. But we assume the index type bit-width is set by kInternalStorageBitWidth, which is already used in the several parts.

As the name suggests, it is supposed to be used for internal storage. Please don't use it beyond that. Using the DataLayout is the right approach, and we do have platforms on which index is lowered to a type with a smaller width than the internal storage.

This revision is now accepted and ready to land.Mar 22 2023, 3:24 AM
bondhugula marked an inline comment as done.

Rebase and add a comment.

This revision was landed with ongoing or failed builds.Mar 22 2023, 3:58 AM
This revision was automatically updated to reflect the committed changes.

MSAN issue/bug introduced by this fixed in https://reviews.llvm.org/D146698