Depends on D124023
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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.
LGTM but I'm not really an authority on the integer arithmetic...
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.
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 | Can you put comment blocks between each op implementation, like this: https://github.com/llvm/llvm-project/blob/a7b154aa1770a2223be2e99735fab54c1c081007/mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp#L190 |
(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. |
@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
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 |
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) |
is this line <= 80 columns wide?