[Mlir-commits] [flang] [mlir] [Flang][OpenMP] Support lowering of simd reductions (PR #112194)

[Sergio Afonso]

skatrak wrote:

> This is to present a uniform way in the flang compiler to message to the user that a feature is not implemented. As you know the user does not care about which part of the compiler does not support the feature. Lowering is currently the place where this is implemented for all features including OpenMP features and we have full control over this.

I'll try to explain a bit better my concern with triggering TODOs in Flang lowering for everything that's not fully supported. Let's say we define the MLIR representation for a given clause. At that point we should be unblocking two lines of work: translation of that clause to LLVM IR, as part of operations that accept it, and Flang lowering to MLIR of that clause and attaching it to the applicable operations. These are two independent features that both need to come together to actually support the new clause by Flang.

However, if we must raise TODO errors in Flang lowering for that clause until it is implemented in both places, then we're artificially creating a dependency. We aren't able to implement and test the lowering to MLIR in Flang because we can't remove the TODO until the other part is also implemented. My thinking is that, by triggering the errors at the point we're actually missing an implementation, we're able to work on both parts independently, since the "contract" between them is the definition of the MLIR operation / clause. From a user perspective, from where we trigger errors doesn't matter, as long as error messages convey the same information.

> Yes, we should provide appropriate errors in translation if a feature/clause/directive/option is not supported. Adding a similar TODO macro to the MLIR to LLVM IR translation would also work. But I am not sure whether it will be accepted since it is not the way things are done in MLIR. But since at the moment it is only Flang work that uses this it might be OK.
> 
> We can see the worst-case issue in this distribute construct translation issue where it presents as a crash. #109755

Looking a bit into this crash, I think the problem here is not only that we introduced an `omp.distribute` operation for which we don't have lowering to LLVM IR implemented yet, but that we're also not checking in the OMPIRBuilder whether the callbacks we're calling succeeded before continuing. I believe the crash would go away if we checked this, and be left with just the error messages ("unsupported OpenMP operation: omp.distribute", "LLVM Translation failed for operation: omp.distribute", ...) and a non-zero exit code. The error propagation is already there in the ModuleTranslation and OpenMPToLLVMIRTranslation, we're just missing some spots in OMPIRBuilder to make sure the compiler doesn't crash when an error is triggered somewhere in the process.

Adding the macro would be mostly to ensure every TODO message is formatted the same way, possibly like Flang lowering, but it's not really necessary if printing the compiler source location of where the error was triggered or defining it as a macro goes against conventions in MLIR.

> Would it be possible to catch this in the call to the translation and issue a TODO in Flang codegen or Driver?

I can't think of a way to do that. We can detect whether translation to LLVM IR failed and then raise an error, but we wouldn't know if it was a TODO that we hit or for what operation it was. I think the functionally equivalent version to that would be to fix the current error handling issues in the translation stage and update error messages to state that it's not yet implemented.

One thing I'd like to propose is to merge this PR so that there is an error triggered for 'simd' but not for 'do simd' reductions (even if it's not a Flang lowering TODO), and work on improving the state of error propagation in the MLIR to LLVM IR translation stage to prevent compiler crashes. This has to be done either way eventually, but it also perhaps helps with concerns of letting Flang lowering produce MLIR that is potentially not yet translatable to LLVM IR.

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