This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Transform PadTensorOp into InitOp, FillOp, GenericOp
ClosedPublic

Authored by agostini01 on May 19 2021, 1:50 PM.

Details

Summary

Introduces a test pass that rewrites PadTensorOps with static shapes as a sequence of:

linalg.init_tensor // to create output
linalg.fill        // to initialize with padding value
linalg.generic     // to copy the original contents to the padded tensor

The pass can be triggered with:

  • --test-linalg-transform-patterns="test-transform-pad-tensor"

Diff Detail

Event Timeline

agostini01 created this revision.May 19 2021, 1:50 PM
agostini01 requested review of this revision.May 19 2021, 1:50 PM

Added missing files for the path

Very cool, thanks for your contribution!
@hanchung @springerm FYI

I'll review tomorrow.

Thank you!
Really appreciate it!
Let me know if any changes are needed.

nicolasvasilache accepted this revision.May 25 2021, 6:55 AM

Approved modulo moving the piece of code that looks like a pad helper.
Thanks for your contribution!

mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
860

s/linalg.generics/linalg.generic/

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
654

Not sure whether you'd need this in a (near) future but I think this can also be generalized by using subtensor_insert:

linalg.init_tensor // to create output
linalg.fill // to initialize with padding value
linalg.subtensor_insert // to copy the original contents to the padded tensor

You could add a simple affine.apply op that does dim(i) + low(i) + high(i) and handle all cases ?
If you did this with makeComposedAffineApply, you'd even get the static shape out of the box and you could branch to the static impl.

This could even turn into a fill + subtensor_insert canonicalization down the line.

659

This seems like a useful function to have as a helper on the pad op ?

686

Ah ok I see why you limit yourself to static cases in this revision, makes sense.

This revision is now accepted and ready to land.May 25 2021, 6:55 AM
  • Fix typos addressing comments
  • Add subtensor_insert comment on where it can replace linalg.generic
agostini01 edited the summary of this revision. (Show Details)May 25 2021, 9:03 AM
agostini01 edited the summary of this revision. (Show Details)
agostini01 accepted this revision.May 25 2021, 9:06 AM
agostini01 marked 4 inline comments as done.

Thank you for the suggestions. I have made small changes to code comments.

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
646

s/with with/with/

654

The use of subtensor_insert is an interesting suggestion. I will experiment with it and submit a new patch if my personal end-to-end examples work with it.

659

I decided against implementing it as a helper function because I see this pattern (cast to shape, hasStaticShape) being used all the time in other parts of the codebase.

684

Added this comment as a reminder that code below can be replaced by subtensor_insert.

agostini01 marked 2 inline comments as done.Jun 1 2021, 10:44 AM

@nicolasvasilache, @mehdi_amini it is the first time I will land a patch through phabricator.

After checking out the branch for this feature I executed:

git pull --rebase https://github.com/llvm/llvm-project.git main
cmake --build /working_dir/llvm-project/build/ --target mlir-opt --target check-mlir # with no failed tests
arc land

Which produce the following output

 STRATEGY  Merging with "squash" strategy, the default strategy.
 SOURCE  Landing the current branch, "lower-pad-tensor".
 ONTO REMOTE  Landing onto remote "origin", the default remote under Git.
 ONTO TARGET  Refs were selected by reading "arc.land.onto" configuration: main.
 INTO REMOTE  Will merge into remote "origin" by default, because this is the remote the change is landing onto.
 INTO TARGET  Will merge into target "main" by default, because this is the "onto" target.
 TARGET  No local copy of ref "main" in remote "origin" exists, attempting fetch...

  $   git fetch --no-tags --quiet -- origin main


 INTO COMMIT  Preparing merge into the empty state to create target "main" in remote "origin".

 <!> LARGE WORKING SET 
There are 23,664 commit(s) reachable from the specified sources
("lower-pad-tensor"). You are landing into the empty state, so all of these
commits will land:

  7d26d6a1b062 Sema: add support for `__attribute__…
  c3fd2a50ba13 [libc] Remove special case for 8 and 16 bytes
  609f5e050cea [mlir] Rename 'setInsertionPointAfter' to avoid ambiguity
  9e3842d60351 [OPENMP]Fix codegen for is_device_ptr component, captured by…
  c3e6054b07be [OpenMP] Additional Information for Libomptarget Mappings
  < ... 23,654 more commits ... >
  0d6e32dcc207 [MLIR][LINALG] Transform PadTensorOp in TensorInitOp and…
  7c89360749fc [MLIR][LINALG] Replace init with GenericOp by FillOp on…
  cd5c8e72c5ab [mlir][linalg] Transform PadTensorOp into InitOp, FillOp…
  fc9fd24e2ba5 Fix typos addressing comments
  db8041a7b8f5 Add subtensor_insert comment on where it can replace linalg.
  >>>  Land 23,664 commit(s)? [y/N/?]

I interrupted this command.
Is arc land the correct command to land these changes?
LLVM docs mention git push.
Should I use git push? Are there arguments I should pass so that the commit message that describes this differential revision are sent in squash mode?
Sorry for the trivial question, just wanted to make sure.

springerm added a comment.EditedJun 3 2021, 12:50 AM

Is arc land the correct command to land these changes?
LLVM docs mention git push.
Should I use git push? Are there arguments I should pass so that the commit message that describes this differential revision are sent in squash mode?
Sorry for the trivial question, just wanted to make sure.

I never tried arc land. I just checkout the main branch, do a git pull origin main, checkout a new branch (git checkout -b br1), cherry-pick over the commit that I want to land, do a git log to see if everything is OK, then push to main (git push origin HEAD:main).

Also, do you have push rights to the repo yet? If this is your first commit, then probably not. In that case, someone else must push the commit on your behalf and you can ask for push rights after showing a good commits. See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

Thank you for the explanation, now I know what I must do.

Also, do you have push rights to the repo yet? If this is your first commit, then probably not. In that case, someone else must push the commit on your behalf and you can ask for push rights after showing a good commits. See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

But you are right, this is my first commit to the llvm-project. It would be great if you or another subscriber for this patch could land it for me.

Below is my name and email as suggested on the instructions:
Nicolas Agostini
n.b.agostini@gmail.com

This revision was landed with ongoing or failed builds.Jun 3 2021, 6:13 AM
This revision was automatically updated to reflect the committed changes.