rjmccall added a reviewer: ABataev.
rjmccall added a comment.
This mostly looks good, but I'd like Alexey Bataev to sign off, since he
authored the OpenMP support.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2124
/*Name=*/".bound.zero.addr");
- CGF.InitTempAlloca(ZeroAddrBound, CGF.Builder.getInt32(/*C*/ 0));
+ CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddrBound);
llvm::SmallVector<llvm::Value *, 16> OutlinedFnArgs;
----------------
This seems innocuous since `ZeroAddrBound` does not otherwise change in the
function, assuming the runtime call doesn't modify it.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:4784
+ CGF.Builder.CreateStore(llvm::ConstantInt::get(CGF.IntPtrTy, 0),
+ NumLVal.getAddress(CGF));
llvm::Value *PrevVal = CGF.EmitLoadOfScalar(NumLVal, E->getExprLoc());
----------------
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.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1503
/*Name=*/".zero.addr");
- CGF.InitTempAlloca(ZeroAddr, CGF.Builder.getInt32(/*C*/ 0));
+ CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddr);
llvm::SmallVector<llvm::Value *, 16> OutlinedFnArgs;
----------------
This seems innocuous since `ZeroAddr` does not otherwise change in the
function, assuming the runtime call doesn't modify it.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:3491
/*Name=*/".zero.addr");
- CGF.InitTempAlloca(ZeroAddr, CGF.Builder.getInt32(/*C*/ 0));
+ CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddr);
// Get the array of arguments.
----------------
This seems innocuous since `ZeroAddr` does not otherwise change in the
function, assuming the runtime call doesn't modify it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111316/new/
https://reviews.llvm.org/D111316
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
