Feature/lospn2std opt select #61
Conversation
LoSPN Categorical leaves are lowered to SelectOp if they consist of two probabilities.
- ADDED support for marginals when lowering from lospn to cpu - ADDED/FIXED support for log-computations - FIXED argument types of SPNSelectLeaf
- Test if lospn.select is replaced by llvm.select (scalar and vectorized)
There was a problem hiding this comment.
Mostly looks good, but the lowering tests need improvement:
- The tests should be more focused on the select and contain less other operations, so it is easier to identify the actual interesting part.
- The tests should cover a scenario where the input type to the leaf is different from it's result type, e.g. taking
i32as input, but producingf64.
For the remaining nits, please see the inline comments.
mlir/lib/Conversion/LoSPNtoCPU/Vectorization/VectorizeNodePatterns.cpp
Outdated
Show resolved
Hide resolved
mlir/lib/Dialect/LoSPN/LoSPNOps.cpp
Outdated
| op.supportMarginalAttr()); | ||
| return success(); | ||
| } | ||
| return failure(); |
There was a problem hiding this comment.
If possible, provide a message on failure by returning rewriter.notifyMatchFailure(...)
There was a problem hiding this comment.
ACK, an explanation is now provided, regarding the rewrite conditions.
mlir/lib/Dialect/LoSPN/LoSPNOps.cpp
Outdated
| return success(); | ||
| } | ||
| } | ||
| return failure(); |
There was a problem hiding this comment.
If possible, provide a message on failure by returning rewriter.notifyMatchFailure(...)
There was a problem hiding this comment.
ACK, an explanation is now provided, regarding the rewrite conditions.
|
Thanks for your review.
The tests are now very concentrated on these instructions, I think they are far better now (and far easier to understand).
I thought that would be covered by |
There was a problem hiding this comment.
Thanks for the update. I found one minor issue in the code, see inline comment.
Other than that, we should also make sure that the optimization is also covered by the Python tests that also check the result of the computation.
Please make sure that
- not all Categorical or Histogram in the existing are replaced by the new optimization
- for every configuration (vectorization, GPU, ...) at least one Categorical and Histogram is replaced and check that the result is computed correctly.
| // Convert from floating-point input to integer value if necessary. | ||
| // This conversion is also possible in vectorized mode. | ||
| auto intVectorTy = VectorType::get(vectorType.getShape(), inputTy); | ||
| thresholdVec = rewriter.create<FPToSIOp>(op->getLoc(), thresholdVec, intVectorTy); |
There was a problem hiding this comment.
Why convert a constant value? thresholdVec is vector of constant values, so it should not be created as vector of floats and then converted, but rather should be created as a vector integers in the first place.
There was a problem hiding this comment.
Absolutely right, it'll be fixed.
ACK.
But all "2 probability/bucket Categoricals/Histograms" should be replaced, right? |
Yes.
Correct, there should be Cat/Hist that are not eligible for the transformation.
Please adapt the existing ones, so they are not eligible for the transformation and add new tests for the transformation. |
- Now, they will not(!) be eligible for "select-transformation". (New tests will be added in the next commit.)
- Added corresponding scalar and vectorized codegen - Adapted regression tests - Added Python computation tests
Optimization feature which will replace certain Categorical and Histogram leaves with Select operations.