Page MenuHomePhabricator

[mlir] Apply source materialization in case of transitive conversion
ClosedPublic

Authored by ftynse on Jan 28 2021, 8:43 AM.

Details

Summary

In dialect conversion infrastructure, source materialization applies as part of
the finalization procedure to results of the newly produced operations that
replace previously existing values with values having a different type.
However, such operations may be created to replace operations created in other
patterns. At this point, it is possible that the results of the _original_
operation are still in use and have mismatching types, but the results of the
_intermediate_ operation that performed the type change are not in use leading
to the absence of source materialization. For example,

%0 = dialect.produce : !dialect.A
dialect.use %0 : !dialect.A

can be replaced with

%0 = dialect.other : !dialect.A
%1 = dialect.produce : !dialect.A  // replaced, scheduled for removal
dialect.use %1 : !dialect.A

and then with

%0 = dialect.final : !dialect.B
%1 = dialect.other : !dialect.A    // replaced, scheduled for removal
%2 = dialect.produce : !dialect.A  // replaced, scheduled for removal
dialect.use %2 : !dialect.A

in the same rewriting, but only the %1->%0 replacement is currently considered.

Change the logic in dialect conversion to look up all values that were replaced
by the given value and performing source materialization if any of those values
is still in use with mismatching types. This is performed by computing the
inverse value replacement mapping. This arguably expensive manipulation is
performed only if there were some type-changing replacements. An alternative
could be to consider all replaced operations and not only those that resulted
in type changes, but it would harm pattern-level composability: the pattern
that performed the non-type-changing replacement would have to be made aware of
the type converter in order to call the materialization hook.

Diff Detail

Unit TestsFailed

TimeTest
330 msx64 debian > LLVM.Transforms/Coroutines::coro-inline.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/Coroutines/coro-inline.ll -always-inline -barrier -coro-split -S -enable-new-pm=0 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/Coroutines/coro-inline.ll
280 msx64 debian > LLVM.Transforms/SampleProfile::cold-indirect-call.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/cold-indirect-call.ll -sample-profile -sample-profile-file=/mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/Inputs/cold-indirect-call.prof -S | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/cold-indirect-call.ll
280 msx64 debian > LLVM.Transforms/SampleProfile::early-inline.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/early-inline.ll -instcombine -sample-profile -sample-profile-file=/mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/Inputs/einline.prof -S | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/early-inline.ll
310 msx64 debian > LLVM.Transforms/SampleProfile::entry_counts_cold.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/entry_counts_cold.ll -sample-profile -sample-profile-file=/mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/Inputs/entry_counts_cold.prof -S | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/entry_counts_cold.ll
300 msx64 debian > LLVM.Transforms/SampleProfile::entry_counts_missing_dbginfo.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/entry_counts_missing_dbginfo.ll -sample-profile -sample-profile-file=/mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/Inputs/entry_counts_cold.prof -S -profile-sample-accurate | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/SampleProfile/entry_counts_missing_dbginfo.ll
View Full Test Results (42 Failed)

Event Timeline

ftynse created this revision.Jan 28 2021, 8:43 AM
ftynse requested review of this revision.Jan 28 2021, 8:43 AM
ftynse updated this revision to Diff 319888.Jan 28 2021, 8:45 AM

Drop unused stuff

If I understand correctly, this revision ensures that we check intermediate values (that did not have a result type change) that still have live uses?

As for the cost of computing the inverse mapping, have you checked if there is an impact to the compile time of any of our large type changing conversions?

If there is no compile time hit, I'm fine with this as is for now.

mlir/lib/Transforms/Utils/DialectConversion.cpp
2228–2264

Can you use an Optional and construct it inside of the loop if necessary? i.e. something like ^

ftynse updated this revision to Diff 320718.Feb 2 2021, 2:03 AM
ftynse marked an inline comment as done.

Review

ftynse added a comment.Feb 2 2021, 3:06 AM

If I understand correctly, this revision ensures that we check intermediate values (that did not have a result type change) that still have live uses?

Yes.

As for the cost of computing the inverse mapping, have you checked if there is an impact to the compile time of any of our large type changing conversions?

If there is no compile time hit, I'm fine with this as is for now.

We are not hitting this code path in any conversion before the child commit, which triggers segfaults on the error path without this change. So, with the optional computation of the inverse map you suggested, there is no effect. I tried with a synthetic test consisting of 10k repetitions of the success test, and see around 9% total time increase in mlir-opt. However, profiling indicates that we spend more time creating cast operations than we spend computing the inverse map.

rriddle accepted this revision.Feb 3 2021, 12:21 PM
This revision is now accepted and ready to land.Feb 3 2021, 12:21 PM
This revision was landed with ongoing or failed builds.Feb 4 2021, 2:15 AM
This revision was automatically updated to reflect the committed changes.