Page MenuHomePhabricator

[OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN
Needs ReviewPublic

Authored by saiislam on Aug 17 2020, 11:55 AM.

Details

Summary

Provide support for amdgcn specific global variables and attributes.
Generalize allocation of various common global variables and provide
their specialized implementations for nvptx and amdgcn.

Diff Detail

Event Timeline

saiislam created this revision.Aug 17 2020, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 11:55 AM
saiislam requested review of this revision.Aug 17 2020, 11:55 AM
ABataev added inline comments.Aug 17 2020, 12:19 PM
clang/lib/CodeGen/CGOpenMPRuntime.h
499

Cab this type and corresponding functions be made AMDGCN-specific only?

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp
116

Is this possible?

119

FlatAttrEmitted

129

CompileTimeThreadLimit

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1394–1395

Restore original formatting.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.h
250–278

Make them protected, not public if possible. Try the same for other new functions.

clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.h
40–65

No need to add virtual, override is enough

saiislam updated this revision to Diff 287513.Aug 24 2020, 3:31 PM
  1. Moved amdgcn specific functions to CGOpenMPAMDGCN.cpp
  2. Removed tautology condition
  3. Corrected case of local variables
  4. Restored original formatting
  5. Changed back declaration of emit kernel methods as private
  6. Added support of amdgcn specific PrePostActionTy implementation and its corresponding test cases
  7. Changed static line numbers in new test cases with regex
  8. Other small code corrections

Reformat the code

clang/lib/CodeGen/CGOpenMPRuntime.h
498

Remove unnecessary formatting changes.

2479–2483

Better to make it a protected member function if you really require it. Plus, this function is very small and, I think, you simply create your own copy in CGOpenMPRuntimeAMDGCN

2487

Same here, make it protected or just create a copy, if it is small.

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h
29–31

Add comments for all new members

117
  1. Do you really need to make this class public?
  2. final
saiislam updated this revision to Diff 288072.Aug 26 2020, 12:16 PM
  1. Reformarting
  2. Comments
  3. Reduced scope of specialized PrePostActionTy
saiislam marked an inline comment as done.Aug 26 2020, 12:27 PM
saiislam added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.h
2479–2483

Not making it protected because it is used by various static functions. And don't want to create an object pointer of subclass of CGOpenMPRuntime in CGOpenMPRuntime.

2487

It calls static functions which in turn call other static functions, so it won't make sense to create a copy of whole function chain in amdgcn.

ABataev added inline comments.Sep 15 2020, 7:44 AM
clang/lib/CodeGen/CGOpenMPRuntime.h
498

Still not removed

684

Restore original formatting

2482–2487

Better to encapsulate these functions into a new utility class and make them public static.

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h
30–63

Do you really need to expose all these new members as public?

31

Runtime does not support nested parallelism on GPU. Do you really need it?

99

It does not help to understand the functionality

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1957

It leads to a mem leak.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.h
37
  1. Make it private or protected

2.Add default initializer

222–254

Are all these required to be public?

420–422

Make it private or protected

saiislam updated this revision to Diff 298377.Thu, Oct 15, 7:29 AM
  1. Removed unnecessary formatting of untouched code.
  2. Encapsulated addFieldToRecordDecl and createGlobalStruct methods in a class and made them static (triggered change at all calling sites).
  3. Marked most of the member methods of CGOpenMPRuntimeAMDGCN as private (forgot to do same change in nvptx)
  4. Fixed the memory leak
  5. Marked appropriate member variables as protected in CGOpenMPRuntimeGPU
clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp
175

The nvptx emitSPMDKernelWrapper does nothing and the amdgcn one appends some metadata.

How about 'nvptx::generateMetadata(...)' that does nothing and 'amdgcn::generateMetadata(...)` that does this stuff, called from the end of emitSPMDKernel?

197

This metadata generation could be split out from the other changes.

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h
49

I'm not convinced by this abstraction. It looks like amdgcn and nvptx want almost exactly the same variable in each case. The difference appears to be that nvptx uses internal linkage and amdgcn uses weak + externally initialized, in which case we're better off with

bool nvptx::needsExternalInitialization() {return false;}
bool amdgpu::needsExternalInitialization() {return true;}

Or, if the inline ternary is unappealing, amdgcn::NewGlobalVariable(...) that passes the arguments to llvm::GlobalVariable while setting the two fields that differ between the two.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.h
175

Please put this back to the previous location so we can see whether it changed in the diff

saiislam marked 3 inline comments as done.Thu, Oct 15, 12:13 PM
saiislam added inline comments.
clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp
175

It will be then difficult to track what all things are being done differently in the two. So, the common code has been generalized and (no change in nvptx + some changes in amdgcn) has been used as specialization.

clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h
49

I understand what you are suggesting. But, there are multiple such variables where linkage between nvptx and amdgcn are different. Also current style gives flexibility to a future implementation to define these variables in their own way.
What do you think?

clang/lib/CodeGen/CGOpenMPRuntimeGPU.h
175

This movement changes them from private to protected.
I could have just added access specifiers and not move the definitions. It would have simplified the review, but it would have decreased the readability for future.

222–254

Yes, they are being called from outside class.

clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
63

Perhaps (typed into browser):

llvm::GlobalVariable *CGOpenMPRuntimeNVPTX::createGlobal( CodeGenModule &CGM, llvm::ArrayType *Ty, StringRef Name) {
  return new llvm::GlobalVariable(
      CGM.getModule(), Ty, /*isConstant=*/false,
      llvm::GlobalVariable::CommonLinkage, llvm::Constant::getNullValue(Ty),
      Name,
      /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal,
      CGM.getContext().getTargetAddressSpace(LangAS::cuda_shared),
/*isExternallyInitialized*/ true);
}

llvm::GlobalVariable *CGOpenMPRuntimeAMDGCN::createGlobal( CodeGenModule &CGM, llvm::ArrayType *Ty, StringRef Name) {
  return new llvm::GlobalVariable(
      CGM.getModule(), Ty, /*isConstant=*/false,
      llvm::GlobalVariable::WeakAnyLinkage, llvm::Constant::getNullValue(Ty),
      Name,
      /*InsertBefore=*/nullptr, llvm::GlobalVariable::NotThreadLocal,
      CGM.getContext().getTargetAddressSpace(LangAS::cuda_shared),
/*isExternallyInitialized*/ false);
}