metagraph-dev / mlir-graphblas

MLIR tools and dialect for GraphBLAS
https://mlir-graphblas.readthedocs.io/en/latest/
Apache License 2.0
15 stars 6 forks source link

Support the graphblas.throw op #104

Open paul-tqh-nguyen opened 3 years ago

paul-tqh-nguyen commented 3 years ago

It'd be nice to do some runtime checks in our MLIR code.

We could solve these problems with safety checks that use exceptions. Since I was unable to find any MLIR-specific exception throwing functionality, I think the solution is to implement our own graphblas.throw.

seibert commented 3 years ago

In principle, this seems reasonable, but I'm worried / concerned that we won't be able actually catch these exceptions. The mechanics of C++ exceptions are pretty complicated. Can we do a quick test to check the feasibility of this?

seibert commented 3 years ago

OK, had a quick chat with Siu and it seems there are a number of reasons this won't work the way we want it to. C++ exception are a bit fragile and magical, and not going to work across shared library boundaries.

The way a number of other languages (Rust, Go) handle this is forbid exceptions entirely and make operations that can fail return a tuple with a bool for success and the actual return value. Then you can check the success flag and return if it fails. CPython does a similar thing, but setting a global exception pointer with the Exception object.

seibert commented 3 years ago

Also, even aside from the stack unwinding issues, the other challenge with exceptions is memory management and leakage

paul-tqh-nguyen commented 3 years ago

I made partial progress on this ticket in https://github.com/paul-tqh-nguyen/mlir-graphblas/commit/167d4cc96f4ad04d3384680b587a9aee14067c44 prior to reading the concerns mentioned in https://github.com/metagraph-dev/mlir-graphblas/issues/104#issuecomment-894459272. I don't think this is a waste of effort since these changes can be easily turned into a graphblas.comment op that we talked about but never implemented.