[Flang][MLIR] Teach AddAliasTags getFuncArgName to deal with omp::TargetOp's #92036
Conversation
…getOp's
This PR attempts to fix the name retrieval of getFuncArgName inside of AddAliasTags
when used in conjunction with an omp::TargetOp. While omp::TargetOp is function like
it does not implement the FunctionOpInterface, so on the occasions where this pass
invokes getFuncArgName on it, it will crash.
This PR attempts to address that by adding a special case for TargetOp where we find
the map operand corresponding to the block argument and return the name attribute
tied to the map operand.
This issue was found in a downstream test case that unfortunately won't run upstream
yet as far as I am aware, the below is the offending block of code:
!$omp target teams distribute parallel do &
!$omp collapse(2) private(i,ib,ic,i1)
do i1=1,3
do ic=1,3
do ib=1,3
do i=1,3
xgnn(1,1,1,1) = 1
end do
end do
end do
end do
!$omp end target teams distribute parallel do
When -O3 (or anything larger than default) is set, the AddAliasTags pass will be activated
and when we encounter a construct like the above in a function or subroutine we end up
ICE'ng the compiler.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: None (agozillon) ChangesThis PR attempts to fix the name retrieval of getFuncArgName inside of AddAliasTags when used in conjunction with an omp::TargetOp. While omp::TargetOp is function like it does not implement the FunctionOpInterface, so on the occasions where this pass invokes getFuncArgName on it, it will crash. This PR attempts to address that by adding a special case for TargetOp where we find the map operand corresponding to the block argument and return the name attribute tied to the map operand. This issue was found in a downstream test case that unfortunately won't run upstream yet as far as I am aware, the below is a variation of the offending block of code (attempted to reduce it a bit..): When -O3 (or anything larger than default) is set, the AddAliasTags pass will be activated and when we encounter a construct like the above in a function or subroutine we end up ICE'ng the compiler. Full diff: https://github.com/llvm/llvm-project/pull/92036.diff 1 Files Affected:
diff --git a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
index 684aa4462915e..026dfe93d6f11 100644
--- a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
+++ b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
@@ -105,8 +105,21 @@ static std::string getFuncArgName(mlir::Value arg) {
"arg is a function argument");
mlir::FunctionOpInterface func = mlir::dyn_cast<mlir::FunctionOpInterface>(
blockArg.getOwner()->getParentOp());
- mlir::StringAttr attr = func.getArgAttrOfType<mlir::StringAttr>(
- blockArg.getArgNumber(), "fir.bindc_name");
+ mlir::StringAttr attr;
+ if (func) {
+ attr = func.getArgAttrOfType<mlir::StringAttr>(blockArg.getArgNumber(),
+ "fir.bindc_name");
+ } else {
+ if (auto targetOp = mlir::dyn_cast<mlir::omp::TargetOp>(
+ blockArg.getOwner()->getParentOp())) {
+ if (auto mapOp = mlir::dyn_cast<mlir::omp::MapInfoOp>(
+ targetOp.getMapOperands()[blockArg.getArgNumber()]
+ .getDefiningOp())) {
+ attr = mapOp.getNameAttr();
+ }
+ }
+ }
+
if (!attr)
return "";
return attr.str();
|
|
There may be better reviewers for this, so if you do know anyone more appropriate please do feel free to add them to the list! Or alternatively, if you see the PR and wish to help review it please do add yourself as well :-) |
There was a problem hiding this comment.
The code changes look good to me, but I wonder if it is a good idea to run AddAliasTags on TargetOp and MapInfoOp.
The tbaa alias information was carefully thought out for normal fortran, but I haven't put much thought into whether it is valid for OpenMP. There is a known issue when inlining the same "function" multiple times into the same block: #89829, and generally we have to be careful about any sort of inlining.
If you have already thought about how the per-"function" alias trees interact with OpenMPIRBuilder then I think this is okay, but I would be hesitant if this is just a case of "make the pass pipeline work with my new operation".
I have some downstream work to generalize the alias tags pass to more operations, but I haven't gotten chance to work on it for a couple of weeks.
(apologies if I missed some patches that landed last week, I am not fully caught up yet after time off)
In this case I hadn't thought too much about the implications of adding the AddAliasTags pass on TargetOp's/MapInfoOp's, I had just assumed (likely incorrectly) that it would be a reasonable thing to do and was perhaps supposed to work already! But it seems that might not be the case, and would require further thought. Is there a possible fix that would exclude TargetOp's from this pass for the time being if that is a more prudent direction? Or any other recommended directions? |
|
Yes you were right to assume that this should work already. I should have got to this sooner. I think I think |
Thank you very much! That seems like a more appropriate fix for the time being, and we can approach the idea of handling TargetOp in passes like this in the future after some more thought if at all necessary. I'll be on vacation until next Thursday, but I'll have a look at your patch ASAP to check if it resolves the issue (feel free to tag me in it and drop a message with the link here)! After which we can likely close this PR! I'll leave it open for the time being incase of further discussion. |
Arguments to openmp regions should not be treated as dummy arguments. This is particularly unsafe because these openmp blocks will eventually be inlined into the calling function, where they will trivially alias with other values inside of the calling function. This is probably a theoretical issue because the calls to openmp runtime function calls would act as barriers, preventing optimizations that are too aggressive. But a lot more thought would need to go into a bet like that. This came out of discussion on llvm#92036
…92156) Arguments to openmp regions should not be tagged as dummy arguments. This is particularly unsafe because these openmp blocks will eventually be inlined into the calling function, where they will trivially alias with other values inside of the calling function. This is probably a theoretical issue because the calls to openmp runtime function calls would act as barriers, preventing optimizations that are too aggressive. But a lot more thought would need to go into a bet like that. This came out of discussion on #92036
|
Fairly certain this can be closed now, so I am going to close it for the time being. @tblah thank you very much for your help! |
This PR attempts to fix the name retrieval of getFuncArgName inside of AddAliasTags when used in conjunction with an omp::TargetOp. While omp::TargetOp is function like it does not implement the FunctionOpInterface, so on the occasions where this pass invokes getFuncArgName on it, it will crash.
This PR attempts to address that by adding a special case for TargetOp where we find the map operand corresponding to the block argument and return the name attribute tied to the map operand.
This issue was found in a downstream test case that unfortunately won't run upstream yet as far as I am aware, the below is a variation of the offending block of code (attempted to reduce it a bit..):
When -O3 (or anything larger than default) is set, the AddAliasTags pass will be activated and when we encounter a construct like the above in a function or subroutine we end up ICE'ng the compiler.