This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][DISC] Revise ParallelLoopTilingPass with inbound_check mode
ClosedPublic

Authored by linearhit on Jul 5 2021, 8:37 PM.

Details

Summary

Expand ParallelLoopTilingPass with an inbound_check mode.

In default mode, the upper bound of the inner loop is from the min op; in inbound_check mode, the upper bound of the inner loop is the step of the outer loop and an additional inbound check will be emitted inside of the inner loop. This was 'FIXME' in the original codes and a typical usage is for GPU backends, thus the outer loop and inner loop can be mapped to blocks/threads in seperate.

Diff Detail

Event Timeline

linearhit created this revision.Jul 5 2021, 8:37 PM
linearhit requested review of this revision.Jul 5 2021, 8:37 PM
mehdi_amini added inline comments.
mlir/include/mlir/Dialect/SCF/Passes.h
45

Please document the new param

mlir/lib/Dialect/SCF/Transforms/ParallelLoopTiling.cpp
41

(Nit: Indent this to match the first dimension)

I think this is the crux of the change here: the tiles will be all the same size instead of being dynamic. I was wondering if there is a better naming here for the option: right now it is pointing at the inbound check inside the body instead of pointing at the change in the tile size.
Adding Uday to have another opinion here.

bondhugula added inline comments.Jul 9 2021, 9:05 PM
mlir/include/mlir/Dialect/SCF/Passes.td
51–54

The name of this option doesn't really convey what it's doing - in fact, it seems to imply exactly the opposite: it is putting the check in the body instead of in the bound! (i.e., in-body-check instead of in-bound-check).

Please update the various instances of upperbound, etc. - I'd recommend space in between upper and bound.

bondhugula edited reviewers, added: ftynse; removed: bondhugula.Jul 9 2021, 9:05 PM

I think a better name here might be something like use-static-loop-bounds or ensure-static-loop-bounds.

We currently get rid of the affine.min when mapping to gpu. Producing static bounded loops like done here would work, too.

I believe the original comment was more about enabling easy vectorization, which this might not achieve. We are using the loop specialization pass for this, which transforms a loop with an upper bound defined via affine.min into a conditional and two loops. That makes the LLVM vectorizer understand the pattern. That is more for CPU use, though.

Just to be clear, I am not arguing against this change.

nit on the tag in the commit title: What is disc? How does that relate to MLIR upstream (more specifically SCF)?

linearhit marked 2 inline comments as done.Jul 14 2021, 8:08 PM
linearhit added inline comments.
mlir/include/mlir/Dialect/SCF/Passes.h
45

done.

mlir/include/mlir/Dialect/SCF/Passes.td
51–54

done, I rename withInboundCheck into useStaticLoopBounds as Herhut suggests.
"upperbound" is changed to "upper bound"

mlir/lib/Dialect/SCF/Transforms/ParallelLoopTiling.cpp
41

Indent fixed.

linearhit marked 2 inline comments as done.Jul 14 2021, 8:08 PM

I think a better name here might be something like use-static-loop-bounds or ensure-static-loop-bounds.

We currently get rid of the affine.min when mapping to gpu. Producing static bounded loops like done here would work, too.

I believe the original comment was more about enabling easy vectorization, which this might not achieve. We are using the loop specialization pass for this, which transforms a loop with an upper bound defined via affine.min into a conditional and two loops. That makes the LLVM vectorizer understand the pattern. That is more for CPU use, though.

Just to be clear, I am not arguing against this change.

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

nit on the tag in the commit title: What is disc? How does that relate to MLIR upstream (more specifically SCF)?

DISC stands for an end2end dynamic shape compiler, which is doing upstreaming now. Most of the codes will be into mlir-hlo repo, with only a few necessary changes will be submitted to LLVM repo.
Please refer to https://drive.google.com/file/d/1t6Q_VhZVWBhi--fYTxTOLGklIemlKQmV/view?usp=sharing

mehdi_amini accepted this revision.Jul 14 2021, 8:25 PM

nit on the tag in the commit title: What is disc? How does that relate to MLIR upstream (more specifically SCF)?

DISC stands for an end2end dynamic shape compiler, which is doing upstreaming now.

We just don't need to tag anything "DISC" in the commit message, I don't think it brings much value to the reader. Downstream projects in general send patches as they make sense in the context of the MLIR project.

This revision is now accepted and ready to land.Jul 14 2021, 8:25 PM
bondhugula added a subscriber: bondhugula.EditedJul 15 2021, 12:59 AM

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

This is a terminology issue but static would be inaccurate here. Throughout the compiler literature pervasively, bounds where you have symbols that do not depend on data are treated as static. They don't have to be constant nor need to be free of min/max. min(0, N) or max(M, N) is also considered statically predictable the same way as 32*N or M if M and N are treated as symbols. Are you specifically avoiding a min/max? In that case, you can make this more descriptive - no-min-max-bounds? "fixed upper bound" would also be confusing in the pass description.

I also notice multiple instances of "inbound" in the code comments which aren't meaningful.

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

This is a terminology issue but static would be inaccurate here. Throughout the compiler literature pervasively, bounds where you have symbols that do not depend on data are treated as static. They don't have to be constant nor need to be free of min/max. min(0, N) or max(M, N) is also considered statically predictable the same way as 32*N or M if M and N are treated as symbols. Are you specifically avoiding a min/max? In that case, you can make this more descriptive - no-min-max-bounds? "fixed upper bound" would also be confusing in the pass description.

I also notice multiple instances of "inbound" in the code comments which aren't meaningful.

I'm also okay on this.
Just to confirm it, is everybody agree with the name no-min-max-bounds? @herhut @mehdi_amini

In my

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

This is a terminology issue but static would be inaccurate here. Throughout the compiler literature pervasively, bounds where you have symbols that do not depend on data are treated as static. They don't have to be constant nor need to be free of min/max. min(0, N) or max(M, N) is also considered statically predictable the same way as 32*N or M if M and N are treated as symbols. Are you specifically avoiding a min/max? In that case, you can make this more descriptive - no-min-max-bounds? "fixed upper bound" would also be confusing in the pass description.

I also notice multiple instances of "inbound" in the code comments which aren't meaningful.

no-min-max-bounds is fine with me. Thanks!

no-min-max-bounds is fine with me. Thanks!

done!

nit on the tag in the commit title: What is disc? How does that relate to MLIR upstream (more specifically SCF)?

DISC stands for an end2end dynamic shape compiler, which is doing upstreaming now.

We just don't need to tag anything "DISC" in the commit message, I don't think it brings much value to the reader. Downstream projects in general send patches as they make sense in the context of the MLIR project.

done!

linearhit marked an inline comment as done.
linearhit requested review of this revision.Jul 30 2021, 10:57 PM
herhut accepted this revision.Aug 2 2021, 2:49 AM

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

This is a terminology issue but static would be inaccurate here. Throughout the compiler literature pervasively, bounds where you have symbols that do not depend on data are treated as static. They don't have to be constant nor need to be free of min/max. min(0, N) or max(M, N) is also considered statically predictable the same way as 32*N or M if M and N are treated as symbols. Are you specifically avoiding a min/max? In that case, you can make this more descriptive - no-min-max-bounds? "fixed upper bound" would also be confusing in the pass description.

This is also the requirement here. You can have min and max and really any computation as long as such computation is not loop dependent. So min(0, N) would be fine. The current code we have introduces a min between the loop index of the outer loop and a symbol, which I would not consider static.

That said, I am also fine with the current name of the flag.

This revision is now accepted and ready to land.Aug 2 2021, 2:49 AM
linearhit added a comment.EditedAug 3 2021, 10:40 PM

I adopt Herhut's suggenstion and rename it into use-static-loop-bounds

This is a terminology issue but static would be inaccurate here. Throughout the compiler literature pervasively, bounds where you have symbols that do not depend on data are treated as static. They don't have to be constant nor need to be free of min/max. min(0, N) or max(M, N) is also considered statically predictable the same way as 32*N or M if M and N are treated as symbols. Are you specifically avoiding a min/max? In that case, you can make this more descriptive - no-min-max-bounds? "fixed upper bound" would also be confusing in the pass description.

This is also the requirement here. You can have min and max and really any computation as long as such computation is not loop dependent. So min(0, N) would be fine. The current code we have introduces a min between the loop index of the outer loop and a symbol, which I would not consider static.

Thanks @herhut
It seems to me that I don't have the permission to merge , could you help with that?

That said, I am also fine with the current name of the flag.

herhut added a comment.Aug 5 2021, 6:42 AM

Thanks @herhut
It seems to me that I don't have the permission to merge , could you help with that?

Sure. Do you have a preferred name/email for me to use as the author of the patch? Essentially what git log would show on your end. I could not find that information.

Thanks @herhut
It seems to me that I don't have the permission to merge , could you help with that?

Sure. Do you have a preferred name/email for me to use as the author of the patch? Essentially what git log would show on your end. I could not find that information.

Sorry for a late reply, please use "tashuang.zk" <tashuang.zk@alibaba-inc.com>
Thanks.