Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenMP] Remove complex reduction variable support #82497

Closed
wants to merge 1 commit into from

Conversation

TIFitis
Copy link
Member

@TIFitis TIFitis commented Feb 21, 2024

This patch removes the complex reduction variables codegen. There are currently no tests for this, and from playing around with some complex reduction variable test cases the code seems unreachable.

The PR #80343 proposes to migrate reductions codegen to the OMPIRBuilder and it isn't currently well equipped to handle complex variables. This patch would enable PR #80343 to go forward.

This patch removes the complex reduction variables codegen. There are currently no tests for this, and from playing around with some complex reduction variable test cases the code seems unreachable.

The PR llvm#80343 proposes to migrate reductions codegen to the OMPIRBuilder and it isn't currently well equipped to handle complex variables. This patch would enable PR llvm#80343 to go forward.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen clang:openmp OpenMP related changes to Clang labels Feb 21, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 21, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Akash Banerjee (TIFitis)

Changes

This patch removes the complex reduction variables codegen. There are currently no tests for this, and from playing around with some complex reduction variable test cases the code seems unreachable.

The PR #80343 proposes to migrate reductions codegen to the OMPIRBuilder and it isn't currently well equipped to handle complex variables. This patch would enable PR #80343 to go forward.


Full diff: https://github.com/llvm/llvm-project/pull/82497.diff

1 Files Affected:

  • (modified) clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp (+3-12)
diff --git a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
index 299ee1460b3db0..a954ad0b3a5e2c 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
@@ -1653,12 +1653,7 @@ static void emitReductionListCopy(
         break;
       }
       case TEK_Complex: {
-        CodeGenFunction::ComplexPairTy Elem = CGF.EmitLoadOfComplex(
-            CGF.MakeAddrLValue(SrcElementAddr, Private->getType()),
-            Private->getExprLoc());
-        CGF.EmitStoreOfComplex(
-            Elem, CGF.MakeAddrLValue(DestElementAddr, Private->getType()),
-            /*isInit=*/false);
+        llvm_unreachable("OpenMP Complex reduction not handled.");
         break;
       }
       case TEK_Aggregate:
@@ -2232,9 +2227,7 @@ static llvm::Value *emitListToGlobalCopyFunction(
       break;
     }
     case TEK_Complex: {
-      CodeGenFunction::ComplexPairTy V = CGF.EmitLoadOfComplex(
-          CGF.MakeAddrLValue(ElemPtr, Private->getType()), Loc);
-      CGF.EmitStoreOfComplex(V, GlobLVal, /*isInit=*/false);
+      llvm_unreachable("OpenMP Complex reduction not handled.");
       break;
     }
     case TEK_Aggregate:
@@ -2439,9 +2432,7 @@ static llvm::Value *emitGlobalToListCopyFunction(
       break;
     }
     case TEK_Complex: {
-      CodeGenFunction::ComplexPairTy V = CGF.EmitLoadOfComplex(GlobLVal, Loc);
-      CGF.EmitStoreOfComplex(V, CGF.MakeAddrLValue(ElemPtr, Private->getType()),
-                             /*isInit=*/false);
+      llvm_unreachable("OpenMP Complex reduction not handled.");
       break;
     }
     case TEK_Aggregate:

@TIFitis
Copy link
Member Author

TIFitis commented Feb 26, 2024

Ping for review.

@jsjodin
Copy link
Contributor

jsjodin commented Mar 4, 2024

@alexey-bataev do you have any concerns about removing this case?

@alexey-bataev
Copy link
Member

@alexey-bataev do you have any concerns about removing this case?

I think better to add a test for complex reductions rather than removing its support. This simple test fails without complex supports.

#include <complex.h>
int foo() {
  int i;
  int j;
  complex float sum;

  #pragma omp target teams loop reduction(+:sum) collapse(2) \
      bind(parallel) order(concurrent) lastprivate(j) map(tofrom:sum)
  for(i=0; i<10; i++)
    for(j=0; j<10; j++)
      sum += i;

  return 0;
}

@TIFitis
Copy link
Member Author

TIFitis commented Mar 5, 2024

Thanks @alexey-bataev for the test. It does hit the complex scenario.

I'll close this PR and add that test in another PR.

@TIFitis TIFitis closed this Mar 5, 2024
@TIFitis TIFitis deleted the reductions_complex branch March 13, 2024 15:28
@TIFitis
Copy link
Member Author

TIFitis commented Mar 18, 2024

@alexey-bataev I'm facing issues adding the test to Clang tests as it uses the complex.h header files. From what I can tell header files don't seem to be supported. Is there any way to add the test you provided to the Clang tests or a different one which can still check for the complex case?

@alexey-bataev
Copy link
Member

You can try to run preprocessor and then manually reduce the expanded code after preprocessor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants