This is an archive of the discontinued LLVM Phabricator instance.

[OpenMPIRBuilder] Add support for generating kmpc_for_static_fini
AcceptedPublic

Authored by kiranchandramohan on Dec 10 2019, 6:26 AM.

Details

Summary

Add support for generating kmpc_for_static_fini in the OpenMP IRBuilder
This is one of the steps required for being able to generate code for omp for/do loops.

Diff Detail

Event Timeline

jdoerfert accepted this revision.Dec 10 2019, 9:18 AM

LGTM, two comments (one below one inlined). For the record: Later we should not expose these low level calls but for now it makes sense to move the code like this.

Please clang format the patch, we should keep the file clang formatted from the very beginning.

llvm/include/llvm/Frontend/OpenMPConstants.h
81 ↗(On Diff #233077)

Nit: Can we reformat these comments, please.

This revision is now accepted and ready to land.Dec 10 2019, 9:18 AM

Are you planning to address the minor comments and upstream this?

reverse ping

After rebasing, I see some crashes in the tests. That is why i did not submit. I will have a look again and see what is the issue.

I see the same crash without my change when enabling openmp-irbuilder in for_codegen.cpp tests. Command and stack trace below.

Is it OK to submit the change without enabling these tests now? I can rebase and make another patch for review without any tests.

/home/kircha02/pristine/llvm-project/build/bin/clang -cc1 -internal-isystem /home/kircha02/pristine/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-enable-irbuilder -x c++ -triple x86_64-unknown-unknown -emit-llvm /home/kircha02/pristine/llvm-project/clang/test/OpenMP/for_codegen.cpp -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope

Stack dump:
0. Program arguments: /home/kircha02/pristine/llvm-project/build/bin/clang -cc1 -internal-isystem /home/kircha02/pristine/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-enable-irbuilder -x c++ -triple x86_64-unknown-unknown -emit-llvm /home/kircha02/pristine/llvm-project/clang/test/OpenMP/for_codegen.cpp -fexceptions -fcxx-exceptions -o - -fsanitize-address-use-after-scope

  1. /home/kircha02/pristine/llvm-project/clang/test/OpenMP/for_codegen.cpp:560:1: current parser token 'char'
  2. /home/kircha02/pristine/llvm-project/clang/test/OpenMP/for_codegen.cpp:541:6: LLVM IR generation of declaration 'parallel_for'
  3. /home/kircha02/pristine/llvm-project/clang/test/OpenMP/for_codegen.cpp:541:6: Generating code for declaration 'parallel_for'
 #0 0x0000ffff97cc52e4 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/kircha02/pristine/llvm-project/llvm/lib/Support/Unix/Signals.inc:564:22
 #1 0x0000ffff97cc538c PrintStackTraceSignalHandler(void*) /home/kircha02/pristine/llvm-project/llvm/lib/Support/Unix/Signals.inc:625:1
 #2 0x0000ffff97cc3664 llvm::sys::RunSignalHandlers() /home/kircha02/pristine/llvm-project/llvm/lib/Support/Signals.cpp:68:20
 #3 0x0000ffff97cc4cb8 SignalHandler(int) /home/kircha02/pristine/llvm-project/llvm/lib/Support/Unix/Signals.inc:406:1
 #4 0x0000ffff9e19e66c  0x66c llvm::CodeExtractor::findAllocas(llvm::CodeExtractorAnalysisCache const&, llvm::SetVector<llvm::Value*, std::vector<llvm::Value*, std::allocator<llvm::Value*> >, llvm::DenseSet<llvm::Value*, llvm::DenseMapInfo<llvm::Value*> > >&, llvm::SetVector<llvm::Value*, std::vector<llvm::Value*, std::allocator<llvm::Value*> >, llvm::DenseSet<llvm::Value*, llvm::DenseMapInfo<llvm::Value*> > >&, llvm::BasicBlock*&) const
 #5 0x0000ffff9e19e66c /home/kircha02/pristine/llvm-project/llvm/lib/Transforms/Utils/CodeExtractor.cpp:492:48
 #6 0x0000ffff9e19e66c llvm::OpenMPIRBuilder::CreateParallel(llvm::OpenMPIRBuilder::LocationDescription const&, llvm::function_ref<void (llvm::IRBuilderBase::InsertPoint, llvm::IRBuilderBase::InsertPoint, llvm::BasicBlock&)>, llvm::function_ref<llvm::IRBuilderBase::InsertPoint (llvm::IRBuilderBase::InsertPoint, llvm::IRBuilderBase::InsertPoint, llvm::Value&, llvm::Value*&)>, std::function<void (llvm::IRBuilderBase::InsertPoint)>, llvm::Value*, llvm::Value*, llvm::omp::ProcBindKind, bool) /home/kircha02/pristine/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:623:30
 #7 0x0000ffff97454798 clang::CodeGen::CodeGenFunction::EmitOMPParallelDirective(clang::OMPParallelDirective const&) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CGStmtOpenMP.cpp:1485:22
 #8 0x0000ffff929045d4 clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CGStmt.cpp:195:5
 #9 0x0000ffff95e83624 clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt const&, bool, clang::CodeGen::AggValueSlot) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CGStmt.cpp:416:3
#10 0x0000ffff95e631a0 clang::CodeGen::CodeGenFunction::EmitFunctionBody(clang::Stmt const*) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1168:1
#11 0x0000ffff95e63bd0 clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, llvm::Function*, clang::CodeGen::CGFunctionInfo const&) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1330:21
#12 0x0000ffff95ee497c clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:4487:3
#13 0x0000ffff95ee5348 clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:2861:47
#14 0x0000ffff95f05348 clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:2614:5
#15 0x0000ffff95eff9c8 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) (.localalias) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:5295:37
#16 0x0000ffff95efeba4 (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:169:7
#17 0x0000ffff95f083bc clang::BackendConsumer::HandleTopLevelDecl(clang::DeclGroupRef) /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:216:7
#18 0x0000ffff96090b28 clang::ParseAST(clang::Sema&, bool, bool) /home/kircha02/pristine/llvm-project/clang/lib/Parse/ParseAST.cpp:162:49
#19 0x0000ffff95ed4428 clang::ASTFrontendAction::ExecuteAction() /home/kircha02/pristine/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1047:11
#20 0x0000ffff900a7e24 clang::CodeGenAction::ExecuteAction() /home/kircha02/pristine/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:1180:1
#21 0x0000ffff950f10ec clang::FrontendAction::Execute() /home/kircha02/pristine/llvm-project/clang/lib/Frontend/FrontendAction.cpp:944:38
#22 0x0000ffff95ed1148 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /home/kircha02/pristine/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:973:42
#23 0x0000ffff950f0b30 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /home/kircha02/pristine/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:282:38
#24 0x0000ffff9507da34 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /home/kircha02/pristine/llvm-project/clang/tools/driver/cc1_main.cpp:240:40
#25 0x0000ffff94d03088 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /home/kircha02/pristine/llvm-project/clang/tools/driver/driver.cpp:330:20
#26 0x0000000000450714 main /home/kircha02/pristine/llvm-project/clang/tools/driver/driver.cpp:407:26
#27 0x0000000000445da8 __libc_start_main /build/glibc-BeHOFw/glibc-2.23/csu/libc-start.c:325:0
kiranchandramohan updated this revision to Diff 289014.EditedAug 31 2020, 1:05 PM

Addressed formatting comment. Rebased.
Removed test since the OpenMP IRBuilder flow generates more ident structures and enabling test involves changes at a lot of places.

Note: The crashes reported above are not there anymore. I think it was fixed by the nested parallel IRBuilder fixes.

You should not need to update the tests as that will be a lot. We will soonish be able to auto-generate the check lines for the existing tests and updates will be less painful.
For now, there should not even be a need as we might be able to test this via existing tests and check lines, see my comment below.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
2819

We do emit exactly the same code in both cases, don't we?
If so, use the always available OMPIRBuilder unconditionally.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
92

Where is the definition for these?

llvm/lib/Frontend/OpenMP/CMakeLists.txt
7

Do we still have that file?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
2819

As of now there is a minor difference. The IRBuilder static fini generates a new call to kmpc_global_thread_num, the existing flow reuses the value computed by a previous call to kmpc_global_thread_num. Seems to be the same functionality.

<   call void @__kmpc_for_static_fini(%struct.ident_t* @1, i32 %8)
---
>   %31 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @2)
>   call void @__kmpc_for_static_fini(%struct.ident_t* @1, i32 %31)
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
92

Good catch. These are in the OMPConstants.cpp file. But i guess we do not want that file.

So we can generate these from the tablegen and remove their declaration here and in OMPConstants.cpp. Is that the right approach?

llvm/lib/Frontend/OpenMP/CMakeLists.txt
7

I will remove this. Also see comment above.

jdoerfert added inline comments.Sep 1 2020, 8:10 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
2819

ok, do you think you can update the tests here by hand? That would allow to remove the old handling right away.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
92

I think the tablegen way is good *if* we can make it nice. If not, we can keep the constants.cpp.
Nice would be, for example, if we generate the isOpenMPNestingDistributeDirective by checking if the directive name starts with distribute. isOpenMPDistributeDirective is similar, isOpenMPLoopDirective will probably require a flag in the directives, though that seems ok and useful either way.

WDYT?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
2819

I will try.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
92

I was thinking of having these functions as properties and in each directive we can say whether this directive has the property. The property can also be classified (if necessary) as conjunctive or disjunctive and based on that we can generate the function.
Looping through all the directives we can generate these functions. Example given below.

def isOpenMPLoopDirective : Property {

let type = Disjunctive;

}

def OMP_TaskLoop : Directive<"taskloop"> {

let allowedClauses = [
  ...
];
let properties = [isOpenMPLoopDirective];
...

}

def OMP_For : Directive<"for"> {

let allowedClauses = [
  ...
];
let properties = [isOpenMPLoopDirective];

}

jdoerfert added inline comments.Sep 2 2020, 11:11 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
92

That looks reasonable to me