Page MenuHomePhabricator

[MLIR] Add std.assume_align op.
ClosedPublic

Authored by timshen on Feb 10 2020, 8:08 PM.

Diff Detail

Event Timeline

timshen created this revision.Feb 10 2020, 8:08 PM
rriddle requested changes to this revision.Feb 10 2020, 10:39 PM
rriddle added inline comments.
mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
2504

Are these patterns sorted alphabetically/can they be?

2533

nit: Move some of these to temporary variables. The nesting is a bit awkward.

2659

This breaks the ordering.

mlir/lib/Dialect/StandardOps/Ops.cpp
2772

Please use the declarative assembly form instead:
https://mlir.llvm.org/docs/OpDefinitions/#declarative-assembly-format

Should be something like:

let assemblyFormat = "$memref attr-dict `:` type($memref)";
2789

Why is the alignment not specified as an argument in ODS? That would autogenerate a lot of this for you automatically; builders, verification, etc.

This revision now requires changes to proceed.Feb 10 2020, 10:39 PM
ftynse requested changes to this revision.Feb 11 2020, 2:27 AM

Thanks for pushing on this. Having alignment attribute as an ODS argument looks like it would remove a bunch of boilerplate for you.

mlir/include/mlir/Dialect/StandardOps/Ops.td
1654

Can you rather add alignment attribute as ODS argument, i.e. (ins AnyMemRef:$memref, I32Attr align) ? This will remove the need for a custom builder, and give you an auto-generated align() instead of hand-written getAlignment().

mlir/lib/Conversion/StandardToLLVM/ConvertStandardToLLVM.cpp
2504

They are not ordered, AFAIK. We should do it as a separate cleanup.

mlir/lib/Dialect/StandardOps/Ops.cpp
2772

Have you considered a non-dictionary syntax: assume_align 64, %memref ? This should be also possible with declarative assembly.

2789

I made the same comment above, +1!

timshen updated this revision to Diff 243961.Feb 11 2020, 12:43 PM

Address all comments.

timshen marked 9 inline comments as done.Feb 11 2020, 12:45 PM

Done. Used ODS and assemblyFormat syntax. My brain didn't wire up to the simple and new stuff. Thanks!

ftynse accepted this revision.Feb 12 2020, 12:55 AM

Thanks!

My brain didn't wire up to the simple and new stuff.

This is also why we have code review, the project is too big for one person to keep track of all the new shiny features. (Except River maybe).

Pinging @rriddle , can you take another look? Thanks!

mehdi_amini accepted this revision.Feb 14 2020, 9:18 PM
rriddle accepted this revision.Feb 14 2020, 9:30 PM

Looks great thanks!

mlir/lib/Dialect/StandardOps/Ops.cpp
2773

llvm::isPowerOf2_32

?

This revision is now accepted and ready to land.Feb 14 2020, 9:30 PM
This revision was automatically updated to reflect the committed changes.