Add runtime check for Gather Op #3069
Conversation
| "indices of GatherOp is larger than the upper bound"); | ||
| Value compareLowerBound = | ||
| create.math.sge(index.getValue(), zeroIE.getValue()); | ||
| rewriter.create<cf::AssertOp>(loc, compareLowerBound, |
There was a problem hiding this comment.
@chentong319 I know it is not optimized for speed, "anding" both condition and calling assert only once would speed the check up a bit. create.math.andi(compareUpperBound, compareLowerBound).
There was a problem hiding this comment.
Separate the check to provide more accurate error message.
| LiteralIndexExpr zero(0); | ||
| Value compareLowerBound = | ||
| create.math.sge(index.getValue(), zero.getValue()); | ||
| rewriter.create<cf::AssertOp>(loc, compareLowerBound, |
There was a problem hiding this comment.
same here, can use andi.
Signed-off-by: Chen Tong <[email protected]>
Signed-off-by: Chen Tong <[email protected]>
|
Jenkins Linux s390x Build #16269 [push] Add runtime check for Ga... started at 21:42 |
|
Jenkins Linux ppc64le Build #15296 [push] Add runtime check for Ga... started at 22:02 |
|
Jenkins Linux amd64 Build #16267 [push] Add runtime check for Ga... started at 20:42 |
|
Jenkins Linux amd64 Build #16267 [push] Add runtime check for Ga... passed after 1 hr 24 min |
|
Jenkins Linux s390x Build #16269 [push] Add runtime check for Ga... passed after 1 hr 27 min |
|
Jenkins Linux ppc64le Build #15296 [push] Add runtime check for Ga... passed after 2 hr 27 min |
Some onnx op, such as GatherOp and GatherElementsOp, requires the input value to be within certain range. For efficiency, onnx-mlir did not check that with the assumption that everything is correct.
This PR adds runtime check for the purpose of Debugging. The implementation is not the optimized. When the bug in Runtime bound check in llvm-project is fixed, we can use that for this purpose.
I tested this PR with RunONNXModel.py with certain inputs. Not sure how to add these test cases into standard test easily.
Future work: any other runtime check is needed?