[PATCH] D93764: [LoopUnswitch] Implement first version of partial unswitching.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I refined one of my comments. I'm other than that OK with this. Let's wait for a few days and get a second opinion.



================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:671
+    if (!I || L->isLoopInvariant(I))
+      continue;
+
----------------
fhahn wrote:
> jdoerfert wrote:
> > Does code generation handle the case where a loop invariant instruction is inside the loop? With the comment below, this might be changed to `L->contains(I)` instead.
> Loop-invariant instructions will be duplicated outside the loop at the moment (added a test). With 'handling them', do you mean updating the uses inside the loop to use the hoisted instruction? Not sure if that will help a lot in practice, as they should already have been moved out I think and/or GVN/LICM will clean them up.
So, my problem was that I assumed something like X below might be considered "loop invariant". However, the API in `llvm::Loop` does simply perform a `constains` check anyway, which is what I suggested instead. That said, I think it is confusing to call `isLoopInvariant` here because what we want/need is `contains`. If someone later recognizes in `isLoopInvariant` that `X` is not loop variant, we would not put the add in the `toDuplicate` set and fail to create valid code, agreed? (Right now it would only show for GEP but it's the same story.)

```
int A[100];
int X, a = ..., b = ...;
for (...) {
  X = a + b;
  A[i] = X;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764