Page MenuHomePhabricator

[mlir][memref] Missing type conversion in memref.reshape llvm lowering
ClosedPublic

Authored by Hardcode84 on Jul 17 2022, 9:51 AM.

Details

Summary

Shape can be memref of index type, so memref::LoadOp result need to be converted into llvm type.

Diff Detail

Event Timeline

Hardcode84 created this revision.Jul 17 2022, 9:51 AM
Herald added a project: Restricted Project. · View Herald Transcript
Hardcode84 requested review of this revision.Jul 17 2022, 9:51 AM

Shape can be memref of index type

Trying to understand if it can be anything else?

Hardcode84 added a comment.EditedJul 18 2022, 12:11 AM

Trying to understand if it can be anything else?

It can be also a memref of plain integer type (there are tests for i64, but I doubt existing code will work for i32 or other integer types).

ftynse requested changes to this revision.Jul 20 2022, 4:56 AM
ftynse added inline comments.
mlir/lib/Conversion/MemRefToLLVM/MemRefToLLVM.cpp
1163–1169

It somehow feels that this (ab)uses the rewriter API. There is no remapped value for dimSize because it is created in the line above, so this just forces the rewriter to call the target materialization in case of type mismatch. Given the amount of TODO in that part of the rewriter, it is not unlikely that the behavior may change and break this in unexpected ways.

I would rather call the materialization directly from type converter if the loaded type is index. ReshapeOp only supports memref of integer or index anyway.

This revision now requires changes to proceed.Jul 20 2022, 4:56 AM

use materializeTargetConversion directly

ftynse accepted this revision.Jul 21 2022, 1:17 AM
This revision is now accepted and ready to land.Jul 21 2022, 1:17 AM