[flang] Consider bind(c) when lowering calls to intrinsic module procedures #70386
Conversation
procedures When attempting to lower an intrinsic module procedure call, take into account bind(c). Such procedures cannot be lowered as intrinsics since their implementation is external to the module. With this solution, the hardcoded "omp_lib" string can be removed when checking if intrinsic module procedure since all procedure interfaces in that module use bind(c).
razvanlupusoru
requested review from
clementval,
jeanPerier,
klausler,
vdonaldson and
vzakhari
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: Razvan Lupusoru (razvanlupusoru) ChangesWhen attempting to lower an intrinsic module procedure call, take into account bind(c). Such procedures cannot be lowered as intrinsics since their implementation is external to the module. With this solution, the hardcoded "omp_lib" string can be removed when checking if intrinsic module procedure since all procedure interfaces in that module use bind(c). Full diff: https://github.com/llvm/llvm-project/pull/70386.diff 1 Files Affected:
diff --git a/flang/lib/Lower/ConvertCall.cpp b/flang/lib/Lower/ConvertCall.cpp
index bc9426827c3ba1d..82e1ece4efeafe7 100644
--- a/flang/lib/Lower/ConvertCall.cpp
+++ b/flang/lib/Lower/ConvertCall.cpp
@@ -2122,7 +2122,12 @@ genProcedureRef(CallContext &callContext) {
mlir::Location loc = callContext.loc;
if (auto *intrinsic = callContext.procRef.proc().GetSpecificIntrinsic())
return genIntrinsicRef(intrinsic, callContext);
- if (Fortran::lower::isIntrinsicModuleProcRef(callContext.procRef))
+ // If it is an intrinsic module procedure reference - then treat as
+ // intrinsic unless it is bind(c) (since implementation is external from
+ // module).
+ if (Fortran::lower::isIntrinsicModuleProcRef(callContext.procRef) &&
+ !Fortran::semantics::IsBindCProcedure(
+ *callContext.procRef.proc().GetSymbol()))
return genIntrinsicRef(nullptr, callContext);
if (callContext.isStatementFunctionCall())
@@ -2227,8 +2232,7 @@ bool Fortran::lower::isIntrinsicModuleProcRef(
return false;
const Fortran::semantics::Symbol *module =
symbol->GetUltimate().owner().GetSymbol();
- return module && module->attrs().test(Fortran::semantics::Attr::INTRINSIC) &&
- module->name().ToString().find("omp_lib") == std::string::npos;
+ return module && module->attrs().test(Fortran::semantics::Attr::INTRINSIC);
}
std::optional<hlfir::EntityWithAttributes> Fortran::lower::convertCallToHLFIR(
|
|
Do we need a test or is there one already for the omp_lib case? |
Yes - turns out we did need a lowering test for this. I confirmed that this new test fails if I remove the special case for "omp_lib", but passes with my change. |
|
The following functions from |
Nice catch! Those need the bind(c) attribute also. The implementation for these is not in the module - but in the runtime. llvm/openmp/runtime/src |
|
For the record, current processing has a negative string compare (to avoid The public |
Thank you for looking into those other modules also. And again for pointing out the issues with omp_lib module! :) I still think that avoiding either a negative/positive test based on module name is probably best. The bind(c) approach appears to be more general and does solve my concern. And it shouldn't impact any of the other modules since omp_lib was the only one treated specially at the point I resolved this issue. |
|
Thanks, the patch looks fine to me. Users might also use the I have asked a question in the patch that introduced this change. https://reviews.llvm.org/D55148. Maybe you can update there as well. |
Thanks Kiran for reviewing! I was considering adding you as a reviewer initially - but at first it seemed very little related to omp since I was just removing the hardcoded line to omp_lib. But either way, I appreciate you took the time to look! Second, I do see that the module there is missing bind(c) also. I have looked at that module and I see several differences - not just this case. For example, in the F18 module, The fact that the module there is missing bind(c) is not an issue as the one in the F18 modules directory since if it is an user module, it won't be considered as intrinsic. But for anything that is intrinsic, we need to specify some implementation - otherwise we cannot distinguish between what remains to be implemented and what is implemented externally. |
Yes, at some point we have to copy that in (instead of the local omp_lib.f90), atleast for the upstream version installation since the upstream version will use the llvm openmp library. I agree that we have to consolidate the changes and that need not be part of this patch. |
|
@razvanlupusoru I get an error when I try to compile your test to binary: There is no error message about not implemented intrinsic, if I revert your changes. |
|
There is likely a missing |
|
I think there is an issue with the non-HLFIR flow. The HLFIR flow is fine. |
Thank you for letting me know! :) |
…ule procedures (llvm#70386)" This reverts commit 529aeaff4d07857027c54e82c0c1ebb6cc6f63ac. This change causes an error: flang-new -fopenmp flang/test/Lower/OpenMP/omp-lib-num-threads.f90 error: loc("flang/test/Lower/OpenMP/omp-lib-num-threads.f90":13:3): flang/lib/Optimizer/Builder/IntrinsicCall.cpp:1375: not yet implemented: intrinsic: omp_set_num_threads
…ines Revert "[flang] Consider bind(c) when lowering calls to intrinsic module procedures (llvm#70386)" This revert patch needs to be removed when upstream is fixed.
…nsic module procedures (llvm#70386)"" (llvm#246) This reverts commit 5919156. Issue solved in upstream commit: llvm#71101
When attempting to lower an intrinsic module procedure call, take into account bind(c). Such procedures cannot be lowered as intrinsics since their implementation is external to the module.
With this solution, the hardcoded "omp_lib" string can be removed when checking if intrinsic module procedure since all procedure interfaces in that module use bind(c).