This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] [OMPIRBuilder] Create a new datatype to hold the unique target region info
ClosedPublic

Authored by jsjodin on Oct 24 2022, 7:34 AM.

Details

Summary

This patch puts the individual target region information attributes into a struct so that the nested mappings are not needed and passing the information around is simplified.

Diff Detail

Event Timeline

jsjodin created this revision.Oct 24 2022, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 7:34 AM
jsjodin requested review of this revision.Oct 24 2022, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 7:34 AM
jdoerfert accepted this revision.Oct 24 2022, 11:58 AM

LG, one suggestions, not necessary to address

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1826

const & for all of them?

This revision is now accepted and ready to land.Oct 24 2022, 11:58 AM
mikerice added inline comments.Oct 24 2022, 12:26 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3031

Needed StringRef(EntryInfo.ParentName) for this to compile for me.

jsjodin updated this revision to Diff 470490.Oct 25 2022, 7:44 AM

Fixed comments

jsjodin marked 2 inline comments as done.Oct 25 2022, 7:45 AM

I updated the patch to address the comment.

This revision was landed with ongoing or failed builds.Oct 25 2022, 8:16 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 8:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kda added a subscriber: kda.EditedOct 26 2022, 1:19 PM

This may have broken HWASAN buildbot:
https://lab.llvm.org/buildbot/#/builders/236/builds/786

Running a build just before and after to validate guess based on stack.

log snippet:

==956880==ERROR: HWAddressSanitizer: tag-mismatch on address 0xffffc09c0031 at pc 0xaaaab749fd64
READ of size 1 at 0xffffc09c0031 tags: c6/00 (ptr/mem) in thread T0
    #0 0xaaaab749fd64 in djbHash /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/include/llvm/Support/DJB.h:22:24
    #1 0xaaaab749fd64 in llvm::StringMapImpl::FindKey(llvm::StringRef) const /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/lib/Support/StringMap.cpp:142:28
    #2 0xaaaab6ab1128 in find /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/include/llvm/ADT/StringMap.h:225:18
    #3 0xaaaab6ab1128 in lookup /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/include/llvm/ADT/StringMap.h:234:25
    #4 0xaaaab6ab1128 in lookup /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/include/llvm/IR/ValueSymbolTable.h:78:17
    #5 0xaaaab6ab1128 in llvm::Module::getNamedValue(llvm::StringRef) const /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/llvm/lib/IR/Module.cpp:111:58
    #6 0xaaaab7c47d2c in clang::CodeGen::CGOpenMPRuntime::createOffloadEntriesAndInfoMetadata() /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp:3077:18
    #7 0xaaaab7946c3c in clang::CodeGen::CodeGenModule::Release() /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:537:20
    #8 0xaaaab86e223c in (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:286:18
    #9 0xaaaab86de0d4 in clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:306:14
    #10 0xaaaaba028aac in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/Parse/ParseAST.cpp:196:13
    #11 0xaaaab86db730 in clang::CodeGenAction::ExecuteAction() /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:1144:30
    #12 0xaaaab85971c0 in clang::FrontendAction::Execute() /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1055:8
    #13 0xaaaab84ebd70 in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1044:33
    #14 0xaaaab86d43d4 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    #15 0xaaaab42e5520 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/tools/driver/cc1_main.cpp:250:15
    #16 0xaaaab42e07f0 in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/tools/driver/driver.cpp:316:12
    #17 0xaaaab42e01b8 in clang_main(int, char**) /b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm-project/clang/tools/driver/driver.cpp:388:12
    #18 0xffff8d6c73f8  (/lib/aarch64-linux-gnu/libc.so.6+0x273f8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5)
    #19 0xffff8d6c74c8 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x274c8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5)
    #20 0xaaaab42b5c6c in _start (/b/sanitizer-aarch64-linux-bootstrap-hwasan/build/llvm_build_hwasan/bin/clang-16+0x5a85c6c)
kda added a comment.Oct 27 2022, 1:55 PM

The bracket builds have finished and they seem to have a conclusive result. The test starts failing with this patch.

Built at this patch: https://lab.llvm.org/buildbot/#/builders/236/builds/803
Built at prior patch: https://lab.llvm.org/buildbot/#/builders/236/builds/804

vitalybuka reopened this revision.Oct 27 2022, 3:21 PM
This revision is now accepted and ready to land.Oct 27 2022, 3:21 PM
vitalybuka added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3031

StringRef(EntryInfo.ParentName) is ref to the scope-local EntryInfo info, so you have HWASAN report here

mikerice added inline comments.Oct 27 2022, 3:53 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3031

Probably meant to change this to const llvm::TargetRegionEntryInfo &EntryInfo to match OffloadTargetRegionEntryInfoActTy?

jsjodin added inline comments.Oct 27 2022, 6:14 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3031

Yes, I think there was an inconsistency introduced when I was splitting up my patch. I will work on a fix.

The bracket builds have finished and they seem to have a conclusive result. The test starts failing with this patch.

Built at this patch: https://lab.llvm.org/buildbot/#/builders/236/builds/803
Built at prior patch: https://lab.llvm.org/buildbot/#/builders/236/builds/804

When I have a fix, is there a way for me to test my code on the buildbot? I don't seem to be able to reproduce the problem on my machine.

When I have a fix, is there a way for me to test my code on the buildbot? I don't seem to be able to reproduce the problem on my machine.

If you have access to valgrind it can be reproduced with that too:

clang -cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm-bc target_messages.cpp -o target_messages.cpp.tmp-ppc-host.bc -DREGION_HOST

valgrind clang -cc1 -fopenmp -fopenmp-version=45 -x c++ -triple powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm target_messages.cpp -fopenmp-is-device -fopenmp-host-ir-file-path target_messages.cpp.tmp-ppc-host.bc -o - -DREGION_DEVICE

mikerice added inline comments.Oct 28 2022, 11:00 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
1909

Just noticed this debug code left in here.

jsjodin updated this revision to Diff 471610.Oct 28 2022, 11:26 AM

Fix reference error.

What is the procedure to redo a patch that was reverted?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
1909

Good catch! I am updating the patch again, on top of the reference fix.

3031

Probably meant to change this to const llvm::TargetRegionEntryInfo &EntryInfo to match OffloadTargetRegionEntryInfoActTy?

jsjodin updated this revision to Diff 471611.Oct 28 2022, 11:30 AM

Remove debug code.