ABataev added inline comments.
================
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());
----------------
rjmccall wrote:
> 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.
>
>
Ho John, you're right, a bugfix. The change looks good.
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
