Page MenuHomePhabricator

[mlir] Fix bug in copy removal
ClosedPublic

Authored by dfki-ehna on Fri, Sep 4, 1:09 AM.

Details

Summary

A crash could happen due to an illegal copy removal in cases such as

%from = scf.for ...
copy(%from, %to) 
dealloc %to

where copy removal removes the entire scf.for operation with its region, Copy, Dealloc operations. Therefore, all getDefiningOp to find the source of Copy operation are replaced with getAllocationOpInBlock. To test this crash, loop_alloc test is added. check_with_affine_dialect test is also added to check the crash issue in TensorFlow::lhlo-copy-removal (issue: https://github.com/tensorflow/mlir-hlo/issues/1). TensorFlow::lhlo-copy-removal pass is going to be replaced with mlir::copy-removal pass.

Diff Detail

Event Timeline

dfki-ehna created this revision.Fri, Sep 4, 1:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 4, 1:09 AM
dfki-ehna requested review of this revision.Fri, Sep 4, 1:09 AM
bondhugula requested changes to this revision.Fri, Sep 4, 1:21 AM
bondhugula added a subscriber: bondhugula.

Nit: commit summary: to -> due to / during the

mlir/lib/Transforms/CopyRemoval.cpp
42

Please document this field.

44

nullptr otherwise.

mlir/test/Transforms/copy-removal.mlir
326

Nit: addition spell fix.

This revision now requires changes to proceed.Fri, Sep 4, 1:21 AM
dfki-ehna updated this revision to Diff 289890.Fri, Sep 4, 1:34 AM

Address the comments.

dfki-ehna marked 3 inline comments as done.Fri, Sep 4, 1:35 AM

Thanks.

dfki-ehna edited the summary of this revision. (Show Details)Fri, Sep 4, 1:42 AM
bondhugula resigned from this revision.Sat, Sep 5, 2:25 PM

@dfki-ehna Thanks for fixing the posted bug. Please do rely on @herhut or @pifon2a's review - they are I assume more familiar with prior code.

pifon2a accepted this revision.Mon, Sep 7, 1:41 AM
pifon2a added inline comments.
mlir/lib/Transforms/CopyRemoval.cpp
33

const std::pair<Value, Value>&

This revision is now accepted and ready to land.Mon, Sep 7, 1:41 AM
herhut accepted this revision.Mon, Sep 7, 2:24 AM

Can you be a bit more descriptive in the summary of what bug was fixed? If the bug was filed somewhere, reference it. Otherwise please describe a little more.

dfki-ehna updated this revision to Diff 290237.Mon, Sep 7, 3:51 AM

Address the comment.

dfki-ehna edited the summary of this revision. (Show Details)Mon, Sep 7, 4:49 AM
dfki-ehna edited the summary of this revision. (Show Details)Tue, Sep 8, 5:12 AM
This revision was landed with ongoing or failed builds.Tue, Sep 8, 5:20 AM
This revision was automatically updated to reflect the committed changes.