Page MenuHomePhabricator

[mlir] Reuse the code between `getMixed*s()` funcs in ViewLikeInterface.cpp.
ClosedPublic

Authored by pifon2a on Jul 28 2022, 7:01 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Jul 28 2022, 7:01 AM
pifon2a requested review of this revision.Jul 28 2022, 7:01 AM
jreiffers added inline comments.Jul 28 2022, 7:04 AM
mlir/lib/Interfaces/ViewLikeInterface.cpp
183

Technically this is not the same anymore and this argument should be removed if it's safe to do so.

Hardcode84 added inline comments.
mlir/lib/Dialect/Utils/StaticValueUtils.cpp
123

staticValues[idx] == dynamicValueIndicator?

pifon2a updated this revision to Diff 448335.Jul 28 2022, 7:20 AM
pifon2a marked an inline comment as done.

Address the comments.

pifon2a added inline comments.Jul 28 2022, 7:22 AM
mlir/lib/Interfaces/ViewLikeInterface.cpp
183

It can be done, but it needs some modifications for ViewLikeOpInterface.

pifon2a added inline comments.Jul 31 2022, 8:13 AM
mlir/lib/Interfaces/ViewLikeInterface.cpp
183

We cannot drop the op argument.

pifon2a marked an inline comment as done.Jul 31 2022, 9:05 AM

PTAL

jreiffers accepted this revision.Jul 31 2022, 11:56 AM
jreiffers added inline comments.
mlir/include/mlir/Interfaces/ViewLikeInterface.td
240–263

Are the isDynamic... functions still needed?

mlir/lib/Dialect/Utils/StaticValueUtils.cpp
136

It's probably better to just return this instead of the ArrayAttr.

  1. It's easy to do the conversion at the call site
  2. It lets you get rid of the builder argument.
  3. SmallVector is easier to use and more type safe than ArrayAttr.
This revision is now accepted and ready to land.Jul 31 2022, 11:56 AM
pifon2a added inline comments.Jul 31 2022, 12:10 PM
mlir/include/mlir/Interfaces/ViewLikeInterface.td
240–263

yes, they are used in a couple of places

mlir/lib/Dialect/Utils/StaticValueUtils.cpp
136

While I agree with that, let's do that in a follow-up. I just moved the code here.

frgossen added inline comments.
mlir/include/mlir/Interfaces/ViewLikeInterface.td
248–263

Why do these have to be configurable? Is anyone using different placeholders?

pifon2a added inline comments.Aug 4 2022, 10:21 AM
mlir/include/mlir/Interfaces/ViewLikeInterface.td
248–263

No, they don't. This interface should ideally be refactored. Moreover, we can use one placeholder for dynamic sizes,strides and offsets, instead of using -1 for kDynamicSizes, but I guess there will be places where -1 was already hardcoded.