[PATCH] D124967: [SROA] Avoid postponing rewriting load/store by ignoring lifetime intrinsics in partition's promotability checking

[Dmitry Vassiliev via Phabricator via llvm-commits]

slydiman added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1993
+      return true;
+  }
+
----------------
nikic wrote:
> slydiman wrote:
> > nikic wrote:
> > > These already is a check for `isLifetimeStartOrEnd()` and `isDroppable()` below, only difference is that it's after the `RelEnd > Size` check. I think you'd want to add the `isDroppable()` check here as well, and then drop the IntrinsicInst handling below entirely.
> > > 
> > > You can also move the `Use *U = S.getUse();` line before here.
> > I have moved `Use *U = S.getUse();`.
> > Please note I cannot move isDroppable() bacause this block depends on the previous block
> > ```
> > } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(U->getUser())) {
> > ```
> > and such moving will break the current logic.
> Do you mean in case a MemIntrinsic is droppable? This cannot happen ("droppable" here means "assume intrinsic" or similar).
I can move the following code to the beginning 
```
  if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser())) {
    if (II->isLifetimeStartOrEnd()) {
      return true;
    } else if (!II->isDroppable()) {
      return false;
    }
  }
```
The returned value `false` here may be wrong because the following block may do not return anything, prevent executing the block `if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser()))` below and finally the return value must be `true`
```
  } else if (MemIntrinsic *MI = dyn_cast<MemIntrinsic>(U->getUser())) {
    if (MI->isVolatile() || !isa<Constant>(MI->getLength()))
      return false;
    if (!S.isSplittable())
      return false; // Skip any unsplittable intrinsics.
  } 
```
Actually such moving breakes a lot of tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124967