This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Lookup the latest value with a legal type when remapping values.
ClosedPublic

Authored by pifon2a on Dec 11 2020, 9:14 AM.

Details

Summary

The current condition implies that the target materialization will be
called even if the type is the new operand type is legal, but slightly
different. For example, if there is a bufferization pattern that changes memref
layout, then target materialization for an illegal type (TensorType) would be
called.

Diff Detail

Event Timeline

pifon2a created this revision.Dec 11 2020, 9:14 AM
pifon2a requested review of this revision.Dec 11 2020, 9:14 AM
herhut requested changes to this revision.Dec 14 2020, 1:25 AM
herhut added inline comments.
mlir/lib/Transforms/Utils/DialectConversion.cpp
1092–1093

I think this could be if (converter && !converter.isLegal(newOperandType)) and then the computation of desiredtype could be moved inside this block.

1092–1093

This line finds the most recent remapped value that has the desired type. Instead, to be in line with the new below code, it should find the most recent remapped value that has a legal type. This is also the only use of this special search functionality,

This revision now requires changes to proceed.Dec 14 2020, 1:25 AM
pifon2a updated this revision to Diff 311881.Dec 15 2020, 5:57 AM

Address the comments.

pifon2a marked 2 inline comments as done.Dec 15 2020, 5:58 AM
pifon2a retitled this revision from [mlir] Fix the condition for target materialization attempt. to [mlir] Lookup the latest value with a legal type when remapping values..Dec 15 2020, 5:59 AM
pifon2a edited the summary of this revision. (Show Details)
herhut added inline comments.Dec 15 2020, 6:47 AM
mlir/lib/Transforms/Utils/DialectConversion.cpp
108–116

Is this still needed? Could we rip out the desiredType support here?

167

Maybe just call lookupOrDefault here?

173

nit: legalValue?

1054–1055

Can we do an early return here to reduce nesting?

pifon2a updated this revision to Diff 311894.Dec 15 2020, 7:04 AM
pifon2a marked an inline comment as done.

Address more comments.

pifon2a marked 2 inline comments as done.Dec 15 2020, 7:05 AM

Thanks @herhut , now it looks much better.

rriddle added inline comments.Dec 15 2020, 11:10 AM
mlir/lib/Transforms/Utils/DialectConversion.cpp
1059

Sink this down to where it is used.

1073

Remove the semi-colon here.

pifon2a updated this revision to Diff 312125.Dec 15 2020, 11:43 PM
pifon2a marked 2 inline comments as done.

Address the comments

herhut accepted this revision.Dec 16 2020, 12:29 AM

Thanks!

This revision is now accepted and ready to land.Dec 16 2020, 12:29 AM
This revision was landed with ongoing or failed builds.Dec 16 2020, 12:53 AM
This revision was automatically updated to reflect the committed changes.