[PATCH] D109749: Experimental Partial Mem2Reg

[Graham Hunter via Phabricator via llvm-commits]

huntergr added a comment.

In D109749#3078046 <https://reviews.llvm.org/D109749#3078046>, @Meinersbur wrote:

> In D109749#3077297 <https://reviews.llvm.org/D109749#3077297>, @huntergr wrote:
>
>> 1. Although the address is loop invariant, the data isn't.
>
> LICM does scalar promotion (controlled by `-disable-licm-promotion`), as in "promote memory location to register". It doesn't matter whether the value at the location is invariant. Whether this belongs into a pass called "Loop Invariant Code Motion" is a different question.
>
>> 2. For this loop in particular, the store is conditional so might never happen. We *could* add a second boolean reduction to determine whether or not to actually perform a store after the loop, but that's a bit more complicated than just letting mem2reg do what it should.
>
> This patch adds another pass, not make mem2reg do it. LICM currently does not handle conditional control flow for scalar promotion, but it should require much less code to change that. See the use of `isGuaranteedToExecute` in `llvm::promoteLoopAccessesToScalars`.

Sorry, I should have made it more clear -- I'm dropping the new pass and using Roman's suggestion of improving SROA. I have implemented that but found the code a bit messy due to the store -> call separation.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109749