This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Move duplicated function getSingleOpOfType into BlockUtilities.h
AbandonedPublic

Authored by clementval on Sep 21 2021, 3:22 AM.

Details

Summary

The tenmplated function getSingleOpOfType was duplicated in two files.
This function is generic enough that other conversion/anaylsis might use it. In
order to avoid more copy/paste of this function, this patch move it into a new
header files for utility function working on Blocks.

https://llvm.discourse.group/t/where-to-move-utility-function/4330

Diff Detail

Event Timeline

clementval created this revision.Sep 21 2021, 3:22 AM
clementval requested review of this revision.Sep 21 2021, 3:22 AM

Add missing newline

Update header guard

jozefl added a subscriber: jozefl.Sep 21 2021, 5:43 AM
jozefl added inline comments.
mlir/include/mlir/IR/BlockUtilities.h
19

This refactor might be a good opportunity to use "\p block" to refer to the "block" parameter in the function's Doxygen comment.

ftynse added inline comments.Sep 22 2021, 1:01 AM
mlir/include/mlir/IR/BlockUtilities.h
17

Please don't do using namespace in a header.

19

We don't use backslash Doxygen commands in MLIR. Doxygen is also able to render Markdown AFAIK.

22

Please fix the linter (may need to #include Block.h)

ftynse requested changes to this revision.Sep 22 2021, 1:01 AM
This revision now requires changes to proceed.Sep 22 2021, 1:01 AM
clementval marked 2 inline comments as done.

Address review comment

mehdi_amini added inline comments.Sep 22 2021, 9:46 AM
mlir/include/mlir/IR/BlockUtilities.h
24

I don't think static is usual for such function in headers.

26

From the function description, I didn't expect a walk: this will traverse nested region and so could return an operation that isn't directly in this block. That makes it a confusing/dangerous API right now.
Would current client work with: for (auto op : block.getOps<OpType>()) instead?

Remove static modifier + simplify the code and make it closer to the description.

ftynse accepted this revision.Sep 22 2021, 1:47 PM
This revision is now accepted and ready to land.Sep 22 2021, 1:47 PM
rriddle requested changes to this revision.Sep 22 2021, 1:49 PM
rriddle added inline comments.
mlir/include/mlir/IR/BlockUtilities.h
23

Does a method like this pull its weight as a general utility? This feels like a very small code block, which would likely be better resolved by something in STLExtras if we really wanted to simplify it.

auto ops = blocks.getOps<OpType>();
OpType op = ops.empty() ? OpType() : *ops.begin();

^ That already feels very simple.

This revision now requires changes to proceed.Sep 22 2021, 1:49 PM
mehdi_amini added inline comments.Sep 22 2021, 2:38 PM
mlir/include/mlir/IR/BlockUtilities.h
23

OpType op = ops.empty() ? OpType() : *ops.begin();

This does not exactly do the same thing, the method here return nullptr if ops has two elements or more.

rriddle added inline comments.Sep 22 2021, 2:40 PM
mlir/include/mlir/IR/BlockUtilities.h
23

Updated:

auto ops = blocks.getOps<OpType>();
OpType op = llvm::hasSingleElement(ops) ? *ops.begin() : OpType();
clementval added inline comments.Sep 22 2021, 10:28 PM
mlir/include/mlir/IR/BlockUtilities.h
23

At this point it might just makes more sense to just replace the function with the small code snippet where it was used.

clementval marked an inline comment as not done.Sep 23 2021, 5:05 AM
clementval added inline comments.
mlir/include/mlir/IR/BlockUtilities.h
23

From the FIR side, we do not really need this function anymore. Anyway, I'm happy to update this patch to remove the duplicates.

clementval abandoned this revision.Sep 30 2021, 7:40 AM
clementval marked an inline comment as not done.

Not needed for FIR anymore.