This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Implement fusion of linalg.generic operation on tensors.
ClosedPublic

Authored by mravishankar on Feb 6 2020, 2:49 PM.

Details

Summary

The initial implementation of the fusion operation exposes a method to
fuse a consumer with its producer, when

  • both the producer and consumer operate on tensors
  • the producer has only a single result value
  • the producer has only "parallel" iterator types

A new interface method hasTensorSemantics is added to verify that an
operation has all operands and results of type RankedTensorType.

Diff Detail

Event Timeline

mravishankar created this revision.Feb 6 2020, 2:49 PM

Updating with changes to fix build failures

Actually updating the file to fix build errors.

Addressing some clang format issues

rriddle added inline comments.Feb 6 2020, 11:10 PM
mlir/include/mlir/Dialect/Linalg/IR/LinalgTraits.h
228

nit: Can you separate the functor to avoid duplication?

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
328

Please remove all trivial braces.

374

Start sentences with a capital.

403

nit: you could use getOperands()[producerOp.getNumInputs()]

427

This seems like an unnecessary usage of for_each. Why not just write a for loop? You don't even need to do that really. You could just use getResultTypes() directly as it is just an ArrayRef<Type>

nicolasvasilache accepted this revision.Feb 7 2020, 7:57 AM

This looks really great, thanks a lot for pushing this Mahesh!
I think it is an elegant solution that shows how proper usage of the StructuredOps abstraction avoid creating problems that should never have existed in the first place (e.g. carrying broadcast and all the phase ordering issues related to buffer allocation, fusion and erasing unnecessary buffers).

Approving conditioned on addressing all comments.

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
337

trivial braces here and below plz

346

re. AffineMap, note that the current "inversion" procedure is best effort atm and will fail in many cases.
You want to check that the inverse computation succeeeds and return false if not.
Otherwise I suspect you'll crash later.

Example that I think should fail today and may crash your transformation down the line: (i, j) -> (i, i+j)

390

This can fail in a bunch of even simple cases.
Plz see my comment above re. checking it in the precondition.

411

Haven't checked deeply that you don't have an off by 1 here somewhere, please be extra careful but you know better than me :)

514

Thanks for writing this as a pattern that will compose with other patterns!

518

trivial braces here and everywhere please

mlir/test/Dialect/Linalg/fusion-tensor.mlir
92

Very cool, thanks Mahesh!

This revision is now accepted and ready to land.Feb 7 2020, 7:57 AM
mravishankar marked 16 inline comments as done.

Updating after addressing comments

mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
346

Thanks. Forgot to check for permutation. Added that check now. I think that is enough for now. If we need to support check of invertability, we can extend that later.

403

I am not sure about that. I am inserting a range of elements. I think the insert method used needs an iterator for the begin/end.

411

I did hit the off by one. Dont need to worry at erase time. THe first insertion will be fine too. Only need to increment the insert position after each insertion to make sure things are inserted at the right place.

427

Thanks for getResultTypes() (cant keep all the utility methods in my head, but should have checked)

rriddle added inline comments.Feb 7 2020, 10:42 AM
mlir/lib/Dialect/Linalg/Transforms/Fusion.cpp
426

Is there any other location you can use besides unknown? We should try to propagate location as much as possible.

437

fusedBlock = b.createBlock(&fusedOpRegion);

?

522

Are all of these .getOperation calls really necessary?

This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Feb 7 2020, 11:54 AM

The test seems to be failing on aarch64, this is the failure we're seeing on aarch64-linux-gnu:

FAIL: Clang :: CodeGen/stack-clash-protection.c (3099 of 16811)
******************** TEST 'Clang :: CodeGen/stack-clash-protection.c' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clanggb_2RS/llvm_build_dir/bin/clang -target x86_64 -O0 -o /b/s/w/ir/k/recipe_cleanup/clanggb_2RS/llvm_build_dir/tools/clang/test/CodeGen/Output/stack-clash-protection.c.tmp.out /b/s/w/ir/k/llvm-project/clang/test/CodeGen/stack-clash-protection.c -fstack-clash-protection && /b/s/w/ir/k/recipe_cleanup/clanggb_2RS/llvm_build_dir/tools/clang/test/CodeGen/Output/stack-clash-protection.c.tmp.out
--
Exit Code: 1

Command Output (stderr):
--
clang-11: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
gcc: error: unrecognized command line option '-m64'
clang-11: error: linker (via gcc) command failed with exit code 1 (use -v to see invocation)

--

I think this test needs REQUIRES: x86.

I just noticed it's failing on macOS as well with a different error:

FAIL: Clang :: CodeGen/stack-clash-protection.c (3400 of 16811)
******************** TEST 'Clang :: CodeGen/stack-clash-protection.c' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clanghb5vUd/llvm_build_dir/bin/clang -target x86_64 -O0 -o /b/s/w/ir/k/recipe_cleanup/clanghb5vUd/llvm_build_dir/tools/clang/test/CodeGen/Output/stack-clash-protection.c.tmp.out /b/s/w/ir/k/llvm-project/clang/test/CodeGen/stack-clash-protection.c -fstack-clash-protection && /b/s/w/ir/k/recipe_cleanup/clanghb5vUd/llvm_build_dir/tools/clang/test/CodeGen/Output/stack-clash-protection.c.tmp.out
--
Exit Code: 1

Command Output (stderr):
--
clang-11: warning: argument unused during compilation: '-fstack-clash-protection' [-Wunused-command-line-argument]
ld: warning: ignoring file /b/s/w/ir/tmp/t/stack-clash-protection-ccc8da.o, file was built for unsupported file format ( 0x7F 0x45 0x4C 0x46 0x02 0x01 0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 ) which is not the architecture being linked (x86_64): /b/s/w/ir/tmp/t/stack-clash-protection-ccc8da.o
Undefined symbols for architecture x86_64:
  "_main", referenced from:
     implicit entry/start for main executable
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
clang-11: error: linker (via gcc) command failed with exit code 1 (use -v to see invocation)

---