This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Removed tight coupling of BufferPlacement pass to Alloc and Dealloc operations by using MemoryEffectOpInterface queries.
ClosedPublic

Authored by dfki-mako on Apr 28 2020, 4:06 AM.

Details

Summary

The current BufferPlacement implementation tries to find Alloc and Dealloc operations in order to move them. However, this is a tight coupling to standard-dialect ops which has been removed in this CL.

Diff Detail

Event Timeline

dfki-mako created this revision.Apr 28 2020, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 4:06 AM
dfki-mako added a project: Restricted Project.
dfki-mako updated this revision to Diff 260586.Apr 28 2020, 4:10 AM

Removed todo comment.

Harbormaster completed remote builds in B54944: Diff 260585.
herhut accepted this revision.Apr 28 2020, 6:10 AM

Please wait for @mehdi_amini to take a look.

mlir/lib/Transforms/BufferPlacement.cpp
361

Would just return work?

378

Nit: Error messages start lower case.

This revision is now accepted and ready to land.Apr 28 2020, 6:10 AM
pifon2a added inline comments.Apr 28 2020, 8:22 AM
mlir/lib/Transforms/BufferPlacement.cpp
195

nit: add period after "interface".

361

not sure if return would work, but instead of

  if (allocateEffectInstance == effects.end())
        return WalkResult::advance();
  auto allocResult = allocateEffectInstance->getValue().cast<OpResult>();
  placements.emplace_back(
         allocResult, analysis.computeAllocAndDeallocPositions(allocResult));
  return WalkResult::advance();
});

try doing

if (allocateEffectInstance != effects.end()) {
   auto allocResult = allocateEffectInstance->getValue().cast<OpResult>();
   placements.emplace_back(allocResult, 
     analysis.computeAllocAndDeallocPositions(allocResult));
}});

to get rid of WalkResult::advance() .

394

I am a bit confused by these two lines:

OpBuilder builder(alloc.getOwner());
builder.setInsertionPointAfter(positions.getDeallocPosition());

the first line creates a builder and sets its insertion point, the second line changes insertion point again. Why not do that in "one hop"? I think i am just confused.

rriddle added inline comments.Apr 28 2020, 10:41 AM
mlir/lib/Transforms/BufferPlacement.cpp
194

This should still be Operation* and not OpOperand?

200

Please make the comments complete sentences.

361

The walk methods are allowed to have void return, so you can strip out the WalkResult::advance. You only need that if you want to interrupt.

Thanks!
LGTM with the other comments addressed

mlir/lib/Transforms/BufferPlacement.cpp
366

Technically I we could have ops allocating two results, seems like you would only see the first?
We may miscompile in such cases I think, can we not try to move such ops if we see them?

dfki-mako updated this revision to Diff 261220.Apr 30 2020, 7:26 AM
dfki-mako marked 8 inline comments as done.

Addressed reviewer comments.

dfki-mako marked 2 inline comments as done.Apr 30 2020, 7:27 AM
dfki-mako added inline comments.
mlir/lib/Transforms/BufferPlacement.cpp
366

Makes sense to me.

herhut accepted this revision.May 4 2020, 4:41 AM

Thanks, LGTM!

This revision was automatically updated to reflect the committed changes.
dfki-mako marked an inline comment as done.