[PATCH] D109749: Experimental Partial Mem2Reg

Thu Oct 21 08:17:03 PDT 2021

Meinersbur added a comment.

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`.

PartialMemToReg uses isAllocaPromotable to ensure that the target is write-accessible and no bit is needed, why not do the same for LICM?

================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:77-78
+      // looking for captures during partialmem2reg.
+      if ((SI->getValueOperand() == AI ||
+           SI->getValueOperand()->getType() != AI->getAllocatedType()) &&
+          !AllowCaptures)
----------------
This is not a sufficient condition for captures. I doubt that we can detect that something has been generated from a CapturedStmt just be looking at the IR.

================
Comment at: llvm/test/Transforms/Mem2Reg/partial-mem2reg.ll:3
+; RUN: opt -partial-mem2reg -S < %s 2>&1 | FileCheck %s --check-prefix=XFORM
+; RUN: opt -partial-mem2reg -gvn -loop-vectorize -debug-only=loop-vectorize -S < %s 2>&1 | FileCheck %s --check-prefix=VDEBUG
+; REQUIRES: asserts
----------------
This tests too many passes at once

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109749