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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopUnswitch.cpp:671
+    if (!I || L->isLoopInvariant(I))
+      continue;
+
----------------
jdoerfert wrote:
> 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;
> }
> ```
> That said, I think it is confusing to call isLoopInvariant here because what we want/need is contains

Yeah, given that we only call it for instructions anyways, using `contains` seems clearer. Updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764