This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Support walks over regions and blocks
ClosedPublic

Authored by frgossen on Oct 29 2020, 3:40 AM.

Details

Summary

Add specializations for walk to allow traversal of regions and blocks.

Diff Detail

Event Timeline

frgossen created this revision.Oct 29 2020, 3:40 AM
frgossen requested review of this revision.Oct 29 2020, 3:40 AM
jpienaar added inline comments.
mlir/include/mlir/IR/Visitors.h
67

Shouldn't this be walkRegions or walkRegionsOfOperation? (This also feels like it may deserve a different comment as the operations one does invoke the callback on the operation, but with regions and blocks it can't)

frgossen updated this revision to Diff 301596.Oct 29 2020, 6:18 AM

Fix clang-tidy

frgossen updated this revision to Diff 301601.Oct 29 2020, 6:48 AM

Address comment

mlir/include/mlir/IR/Visitors.h
67

Different names for regions, blocks, and operations would mean to also split the template below into 3 variants.
I renamed it to walk (which is also the name used when using this through the IR classes) and tried to clarify in the comments.

frgossen marked an inline comment as done.Oct 29 2020, 6:49 AM
pifon2a accepted this revision.Oct 29 2020, 7:33 AM
This revision is now accepted and ready to land.Oct 29 2020, 7:33 AM
This revision was automatically updated to reflect the committed changes.
rriddle added inline comments.Oct 29 2020, 9:32 AM
mlir/include/mlir/IR/Visitors.h
95

Use llvm::is_one_of here.

frgossen marked an inline comment as done.Oct 30 2020, 3:19 AM
frgossen added inline comments.
mlir/include/mlir/IR/Visitors.h
95

We are seeing a failure in the mlir build on Ubuntu with gcc/g++ since these two changes went in:

FAILED: tools/mlir/lib/Dialect/GPU/CMakeFiles/obj.MLIRGPU.dir/Transforms/AsyncRegionRewriter.cpp.o 
/usr/bin/c++ -DBUILD_EXAMPLES -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/mlir/lib/Dialect/GPU -I/vstsdrive/_work/20/s/llvm-project/mlir/lib/Dialect/GPU -Iinclude -I/vstsdrive/_work/20/s/llvm-project/llvm/include -I/vstsdrive/_work/20/s/llvm-project/mlir/include -Itools/mlir/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -g  -fno-exceptions -std=c++14 -MD -MT tools/mlir/lib/Dialect/GPU/CMakeFiles/obj.MLIRGPU.dir/Transforms/AsyncRegionRewriter.cpp.o -MF tools/mlir/lib/Dialect/GPU/CMakeFiles/obj.MLIRGPU.dir/Transforms/AsyncRegionRewriter.cpp.o.d -o tools/mlir/lib/Dialect/GPU/CMakeFiles/obj.MLIRGPU.dir/Transforms/AsyncRegionRewriter.cpp.o -c /vstsdrive/_work/20/s/llvm-project/mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp
/vstsdrive/_work/20/s/llvm-project/mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp: In member function ‘virtual void {anonymous}::GpuAsyncRegionPass::runOnFunction()’:
/vstsdrive/_work/20/s/llvm-project/mlir/lib/Dialect/GPU/Transforms/AsyncRegionRewriter.cpp:113:16: internal compiler error: in replace_placeholders_r, at cp/tree.c:2804
   if (getFunction()
       ~~~~~~~~~~~~~
           .getRegion()
           ~~~~~~~~~~~~
           .walk(Callback{OpBuilder(&getContext())})
           ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-7/README.Bugs> for instructions.
frgossen marked an inline comment as done.Nov 2 2020, 2:09 AM

Unfortunately, I could not reproduce this failure,
I tried with gcc/g++ (Debian 10.2.0-9) 10.2.0.

Unfortunately, I could not reproduce this failure,
I tried with gcc/g++ (Debian 10.2.0-9) 10.2.0.

This reproduces reliably on Ubuntu 18.04.5 LTS with gcc/g++ 7.5.0

@frgossen , Thanks! Let me know if you need any help investigating.

bkramer added a subscriber: bkramer.Nov 2 2020, 2:35 PM

Building with GCC7 is fixed by 5ece2115d85326e3b24336a7ccb4c191932ccb4a, but looks like this was reverted already.

Yes, I reverted it yesterday as it caused problems with the integrate. I will re-land this when I figure out how to install an old GCC version.

Yes, I reverted it yesterday as it caused problems with the integrate. I will re-land this when I figure out how to install an old GCC version.

Sorry, what I was trying to say was that 5ece2115d85326e3b24336a7ccb4c191932ccb4a fixed the GCC7 crash. Your change seems mostly unrelated and should be safe to reland.