[PATCH] D124967: [SROA] Avoid postponing rewriting load/store by ignoring lifetime intrinsics in partition's promotability checking
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 16 13:33:37 PDT 2022
nikic added inline comments.
================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1993
+ return true;
+ }
+
----------------
slydiman wrote:
> 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.
To be clear, this is what I meant:
```
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(U->getUser())) {
if (II->isLifetimeStartOrEnd() || II->isDroppable())
return true;
}
```
You are returning false for all non-droppable intrinsics, while we only want to return true for droppable intrinsics, and leave everything else to go through the code below (including MemIntrinsic).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124967/new/
https://reviews.llvm.org/D124967
More information about the llvm-commits
mailing list