[PATCH] D109749: Experimental Partial Mem2Reg

[Graham Hunter via Phabricator via llvm-commits]

huntergr added a comment.

In D109749#3085994 <https://reviews.llvm.org/D109749#3085994>, @jdoerfert wrote:

> In D109749#3078247 <https://reviews.llvm.org/D109749#3078247>, @huntergr wrote:
>
>> In D109749#3078176 <https://reviews.llvm.org/D109749#3078176>, @jdoerfert wrote:
>>
>>> Does this work for you:
>>>
>>>   diff --git a/llvm/lib/Analysis/CaptureTracking.cpp b/llvm/lib/Analysis/CaptureTracking.cpp
>>>   index 8955658cb9e7..41251d2676e6 100644
>>>   --- a/llvm/lib/Analysis/CaptureTracking.cpp
>>>   +++ b/llvm/lib/Analysis/CaptureTracking.cpp
>>>   @@ -373,9 +373,13 @@ void llvm::PointerMayBeCaptured(const Value *V, CaptureTracker *Tracker,
>>>        case Instruction::Store:
>>>          // Stored the pointer - conservatively assume it may be captured.
>>>          // Volatile stores make the address observable.
>>>   -      if (U->getOperandNo() == 0 || cast<StoreInst>(I)->isVolatile())
>>>   +      if (U->getOperandNo() == 0 || cast<StoreInst>(I)->isVolatile()) {
>>>   +        if (auto *AI = dyn_cast<AllocaInst>(I->getOperand(1)->stripInBoundsOffsets()))
>>>   +          if (AI->hasMetadata("nocapture_storage"))
>>>   +            break;
>>>            if (Tracker->captured(U))
>>>              return;
>>>   +      }
>>>          break;
>>>        case Instruction::AtomicRMW: {
>>>          // atomicrmw conceptually includes both a load and store from
>>>
>>> And then add `!nocapture_storage !0` after the alloca in your example as well as `!0 = !{!0}` in the end of that file
>>
>> Ah, the 'nocapture_storage' metadata is what I've been missing, thanks. I'll update the diff once I've added that and adjusted the tests.
>
> Technically, this is not yet something we have in the IR. We can reply to the old thread in which different solutions were discussed and
> propose this one again. Then modify Clang to emit the metadata for the reduction case and land the diff I posted. All that said, it works
> for your case, right?

It does, yes. I'll have a look for the mailing list thread.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109749/new/

https://reviews.llvm.org/D109749