Page MenuHomePhabricator

[CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()
ClosedPublic

Authored by hsmhsm on Oct 7 2021, 8:16 AM.

Details

Summary

Sequel patch to https://reviews.llvm.org/D111293.

Remove call to CodeGenFunction::InitTempAlloca() from OpenMP related
codegen part.

Also remove the metadata !llvm.access.group from the updated lit
tests.

Diff Detail

Event Timeline

hsmhsm requested review of this revision.Oct 7 2021, 8:16 AM
hsmhsm created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 8:16 AM
hsmhsm updated this revision to Diff 378093.Oct 7 2021, 10:22 PM

Move OpenMP GPU offloading codegen related changes from https://reviews.llvm.org/D111324 to here.

hsmhsm updated this revision to Diff 378412.Oct 8 2021, 9:47 PM

Rebase.

This mostly looks good, but I'd like Alexey Bataev to sign off, since he authored the OpenMP support.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
2124

This seems innocuous since ZeroAddrBound does not otherwise change in the function, assuming the runtime call doesn't modify it.

4784

The original generated code here is very strange:

%depobj.size.addr = alloca i64
...  
store i64 0, i64* %depobj.size.addr    // currently in prologue
...
%numdeps = load // from the size field of the struct kmp_depend_info which precedes all dependent objects
...
%tmp = load i64, i64* %depobj.size.addr
%sum = add nuw i64 %tmp, i64 %numdeps
store i64 %sum, %i64* %depobj.size.addr
...
// and then in the one place we actually call this function, we
// immediately sum up all the %sums for all the dependent objects

The funny thing about this code is that, because the zeroing of %depobj.size.addr is only done in the prologue, it increases by the current element count of the depend object every time if this code is run multiple times, which I think cannot possibly be correct. Notably, the code to actually copy dependency data does not seem to have this bug because PosLVal is zeroed immediately.

So I'm pretty sure this change is actually a bug fix in the treatment of depend(depobj : ...). However, I'd really like Alexey to review and approve, because if %depobj.size.addr is *not* supposed to accumulate values in a loop, then I really don't understand why it exists at all.

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
1503

This seems innocuous since ZeroAddr does not otherwise change in the function, assuming the runtime call doesn't modify it.

3491

This seems innocuous since ZeroAddr does not otherwise change in the function, assuming the runtime call doesn't modify it.

ABataev added inline comments.Oct 11 2021, 4:06 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
4784

Ho John, you're right, a bugfix. The change looks good.

rjmccall accepted this revision.Oct 11 2021, 2:20 PM

Okay, thanks, Alexey. LGTM, then.

This revision is now accepted and ready to land.Oct 11 2021, 2:20 PM