[Mlir-commits] [llvm] [mlir] [MLIR][OpenMP][OMPIRBuilder] Error propagation across callbacks (PR #112533)

[Sergio Afonso]

skatrak wrote:

> I think continuing on an unrecoverable error is an inherently bad idea. It will create code paths that will never be tested by definition (because if there is a path we will fix the bug leading to that path) and hence likely cause dereferenced null pointers, trigger assertions, etc already because the LLVM generated so far is not in an expected state. For instance, the new early abort in `createTaskgroup` may leave a degenerate BB, any call to `BB::getTerminator()` will return nullptr, and at latest the IR-verifier will crash. Any `llvm_unreachable` and `assert` in OpenMPIRBuilder and elsewhere will also still cause a crash, not `mlir::emitError` which seems inconsequential.
> 
> However, of course I will not stand in the way if this is the community-chosen solution and/or established pattern. Also, if there is use case with a recoverable error we would need this as well.

I agree that once one of these errors were triggered, we'd have to stop any further translation or codegen process. But the approach I'm proposing actually does this. There is indeed some level of collaboration needed at each stage for that to happen, but basically the pattern is already to return `LogicalResult` from translation functions, always check its return value and return a failure straight away if it didn't succeed. So, once an OMPIRBuilder function fails, we can emit an error and return failure. This is checked by the caller, the caller of the caller, etc. until the original `mlir::translateModuleToLLVMIR()` call returns a null pointer instead of a half-created invalid IR.

> I might have overblown the problem. If with every `Error`/`Expected` we immediately return from the entire call stack, until there is no use of the LLVM-IR, there should not be a lot of things that could go wrong. Clang's use of OpenMPIRBuilder still needs to crash on an erroneous OpenMPIRBuilder call because it doesn't have that `Error`/`Expected` design.
> 
> MLIR uses Rust-style error propagation, and since OpenMPIRBuilder is used within MLIR, it should be compatible with it. I am still not convinced that it provides any benefit, but consider my objections retracted.

In a way, the changes to the OMPIRBuilder are not really mandatory to make use of. Callbacks passed to it from Clang can decide to exit the program on irrecoverable failure and always return success, and assume the returned `Expected` values never hold an `Error`. In MLIR this lets us integrate with the existing error reporting pattern, but it also creates the infrastructure to deal with recoverable errors in both Clang and Flang. I see it as a bit more flexible solution.

https://github.com/llvm/llvm-project/pull/112533