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

[Alexey Bataev via Phabricator via cfe-commits]

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