Page MenuHomePhabricator

[mlir](arithmetic) Add ceildivui to the arithmetic dialect
ClosedPublic

Authored by lipracer on Nov 7 2021, 3:04 AM.

Details

Summary

The specific description is Adding unsigned integer ceil in Std Dialect .

When we lower ceilDivOp this will generate below code, sometimes we know m and n are unsigned intergal.Here are some redundant judgments about positive and negative.
So we need to add some unsigned operations to simplify the instructions.

ceilDiv(n, m)
  x = (m > 0) ? -1 : 1
  return (n*m>0) ? ((n+x) / m) + 1 : - (-n / m)

unsigned operations:

ceilDivU(n, m)
  return n ==0 ?  0 :  ((n - 1) / m) + 1

Diff Detail

Event Timeline

lipracer created this revision.Nov 7 2021, 3:04 AM
lipracer requested review of this revision.Nov 7 2021, 3:04 AM
lipracer edited the summary of this revision. (Show Details)Nov 7 2021, 3:06 AM
lipracer edited the summary of this revision. (Show Details)
lipracer updated this revision to Diff 385333.Nov 7 2021, 3:28 AM

Format the code with the latest clang-format.

lipracer retitled this revision from Add ceildivui floordivui to the arithmetic dialect to [mlir](arithmetic) Add ceildivui floordivui to the arithmetic dialect.Nov 7 2021, 6:48 PM
lipracer edited the summary of this revision. (Show Details)

Thanks for the patch!

mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td
288

Is this line longer than 80 characters?

331

Ditto

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
245

Unsigned*

248

Drop the inline

330

Drop the trivial braces

mlir/lib/Dialect/Arithmetic/Transforms/ExpandOps.cpp
96

*FloorDivUIOp

103

It didn't occur to me when I was reading the RFC, but I guess FloorDivUIOp is just an alias for DivUIOp? Is there a compelling reason to actually have a separate op? (other than symmetry :P)

104

rewriter.replaceOpWithNewOp

Mogball requested changes to this revision.Nov 8 2021, 10:13 AM
This revision now requires changes to proceed.Nov 8 2021, 10:13 AM
lipracer added inline comments.Nov 8 2021, 6:00 PM
mlir/lib/Dialect/Arithmetic/Transforms/ExpandOps.cpp
104

Indeed, this is an alias. When I add this op, would it be more acceptable if we can define an Op with an alias? I really don't have a reason to add it.I will remove later.

lipracer updated this revision to Diff 385690.Nov 8 2021, 7:44 PM
lipracer retitled this revision from [mlir](arithmetic) Add ceildivui floordivui to the arithmetic dialect to [mlir](arithmetic) Add ceildivui to the arithmetic dialect.
lipracer edited the summary of this revision. (Show Details)
lipracer marked 6 inline comments as done.
lipracer updated this revision to Diff 385728.Nov 9 2021, 1:11 AM

format code with clang-format-13.0

Mogball accepted this revision.Nov 9 2021, 8:56 AM

LGTM just one small comment and then you're good

mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td
286

Oh this one looks one character over 80 too.

This revision is now accepted and ready to land.Nov 9 2021, 8:56 AM
lipracer added inline comments.Nov 9 2021, 9:04 AM
mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td
286

I have also noticed that the above document also exceeds 80 characters. I wonder if the single quotation mark as a whole can be relaxed?

lipracer updated this revision to Diff 386040.Nov 9 2021, 6:52 PM

update for format error

lipracer marked an inline comment as done.Nov 9 2021, 6:54 PM

nex what I need to do? I don't have permission to commit.

I'll commit this for you. Thanks!

mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td
286

A lot of this was copy-pasted so yeah I should go back and clean this up too.

Thank very much for your feedback.

This revision was automatically updated to reflect the committed changes.