This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Arith] Implement InferIntRangeInterface for arithmetic ops
ClosedPublic

Authored by krzysz00 on Apr 19 2022, 10:57 AM.

Diff Detail

Event Timeline

krzysz00 created this revision.Apr 19 2022, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 10:57 AM
krzysz00 requested review of this revision.Apr 19 2022, 10:57 AM
Mogball added inline comments.
mlir/include/mlir/Dialect/Arithmetic/IR/InferIntRangeInterface.h
35 ↗(On Diff #423677)

Please move these into a source file.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
52 ↗(On Diff #423677)

I would move the definition of the range inference functions to another file to avoid bloating Ops.cpp as much as possible.

rriddle requested changes to this revision.Apr 19 2022, 12:02 PM

Can you add test cases that exercise all of the logic happening here? Maybe it would be better to stage the commits in a way that just the interface+analysis are added first, and new implementations can be added and tested incrementally (using the analysis as the test driver).

mlir/include/mlir/Dialect/Arithmetic/IR/InferIntRangeInterface.h
17–23 ↗(On Diff #423677)

Do you need all of these includes for the header? Can you trim these down to what is necessary?

32 ↗(On Diff #423677)

Can you make this a proper struct? It would make the accessors nicer and more consistent.

35 ↗(On Diff #423677)

+1 Same with the operator<< (to remove the need for several of the includes).

mlir/include/mlir/Dialect/Arithmetic/IR/InferIntRangeInterface.td
1 ↗(On Diff #423677)

Please make sure that the line length is a max of 80 characters.

20–23 ↗(On Diff #423677)

Can you use 2 space indents please? These files seems to be using 4.

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
15 ↗(On Diff #423677)

Are all of these new includes necessary?

51 ↗(On Diff #423677)

Please use /// for top-level comments.

122–126 ↗(On Diff #423677)

These are quite large blocks, can you wrap with braces?

134 ↗(On Diff #423677)

Please end comments with punctuation.

135 ↗(On Diff #423677)

Missing static here?

385 ↗(On Diff #423677)

Can you describe the issue here?

2070 ↗(On Diff #423677)

Missing static here?

Also can you document what this function does?

2716 ↗(On Diff #423677)

nit: Drop the newline here.

This revision now requires changes to proceed.Apr 19 2022, 12:02 PM

Just to give an update: I'm in the process of addressing the feedback here and pulling the inference implementations into a separate file. As I was doing this, I realized there were some potential issues with how I implemented the analysis and that I should refactor it to track unsigned and signed bounds separately (thus improving its precision).

Once that's all dealt with, this revision will depend on D124023, so that we can have the tests for the implementations of InferIntRangeInterface in the same revision as the implementation itself.

krzysz00 updated this revision to Diff 424583.Apr 22 2022, 12:40 PM
krzysz00 retitled this revision from [mlir][Arith] Add InferIntRange interface and implementations to [mlir][Arith] Implement InferIntRangeInterface for arithmetic ops.
krzysz00 edited the summary of this revision. (Show Details)

Reorganize revisions per feedback

krzysz00 updated this revision to Diff 425018.Apr 25 2022, 1:32 PM

Refactor for class encapsulation.

krzysz00 updated this revision to Diff 426781.May 3 2022, 11:00 AM

Update impls for Optional<APInt> change

krzysz00 updated this revision to Diff 428162.May 9 2022, 12:29 PM

Change implementations to account for interface change.

krzysz00 updated this revision to Diff 428507.May 10 2022, 2:57 PM

Account for changes in dependent commits, main

Mogball accepted this revision.May 11 2022, 8:58 PM

LGTM but I'm not really an authority on the integer arithmetic...

@rriddle?

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

is this line <= 80 columns wide?

111

can you add this directly to cmpi instead of both cmpi and cmpf?

975–978
mlir/lib/Dialect/Arithmetic/IR/InferIntRangeInterfaceImpls.cpp
26

Can you add this to the interface header and rename it to SetIntRangeFn?

81–82

drop braces

117

can you spell out the autos for all these lambdas?

Yep. I'm waiting for the design to settle out on the lower layers before reviewing this one.

krzysz00 updated this revision to Diff 429019.May 12 2022, 11:00 AM
krzysz00 marked 6 inline comments as done.

Formatting and review fixes.

krzysz00 updated this revision to Diff 430447.May 18 2022, 10:39 AM

Use non-optional-ness of APInt

krzysz00 updated this revision to Diff 431750.May 24 2022, 12:20 PM

Move tuple wrappers out of older commit.

krzysz00 updated this revision to Diff 433149.May 31 2022, 11:15 AM
krzysz00 edited the summary of this revision. (Show Details)

Update for removal of notAnInteger(), move to SCCP

krzysz00 updated this revision to Diff 433840.Jun 2 2022, 12:32 PM
krzysz00 edited the summary of this revision. (Show Details)

Rebase, change test pass name, add test

krzysz00 updated this revision to Diff 433842.Jun 2 2022, 12:33 PM

Rename test file

rriddle added inline comments.Jun 6 2022, 11:22 PM
mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td
1205

This looks unrelated.

mlir/lib/Dialect/Arithmetic/IR/InferIntRangeInterfaceImpls.cpp
29

Could this just return a ConstantIntRanges?

69
krzysz00 updated this revision to Diff 434893.Jun 7 2022, 11:08 AM
krzysz00 marked an inline comment as done.

Add block comments, remove IntPair

krzysz00 marked an inline comment as done.Jun 7 2022, 11:09 AM

(Submitting inline comments)

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

... Oh. That's trailing whitespace removal. Should I put the spaces back?

mlir/lib/Dialect/Arithmetic/IR/InferIntRangeInterfaceImpls.cpp
29

Agreed, a lot of these functions can return ranges now that I'm looking again.

krzysz00 updated this revision to Diff 435346.Jun 8 2022, 2:49 PM

Rebase onto main, fix tests to account for arith.constant inference

krzysz00 updated this revision to Diff 435358.Jun 8 2022, 3:10 PM

Add a test closer in intent to the original while loop test

@Mogball , I know you LGTM'd this a while back, but it got more-or-less rewritten since then, so it could probably use another style/sanity check

still looks good to me. @rriddle ?

rriddle accepted this revision.Jun 11 2022, 1:00 AM

LG, mostly stylistic comments.

mlir/lib/Dialect/Arithmetic/IR/InferIntRangeInterfaceImpls.cpp
23

You can drop the llvm:: here.

26

Is this something that should be on ConstantIntRanges directly?

75

ConstantIntRanges(value, value, value, value)

Can we have a constructor that abstracts this away?

90–92
97–99

Same as above.

151–153

Same as above for each of these.

185–188

Can we do the same throughout?

363
365–366
376–377
404

Please apply the same throughout.

mlir/lib/Interfaces/InferIntRangeInterface.cpp
40–42 ↗(On Diff #435358)
This revision is now accepted and ready to land.Jun 11 2022, 1:00 AM
krzysz00 updated this revision to Diff 436812.Jun 14 2022, 9:05 AM
krzysz00 marked 5 inline comments as done.

Apply style comments, send off last build pre-land

krzysz00 updated this revision to Diff 436828.Jun 14 2022, 9:47 AM
krzysz00 marked 2 inline comments as done.

unsigned int -> unsigned throughought