[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:742
+          for (Use &U : Current->uses())
+            AccessesToCheck.push_back(cast<MemoryAccess>(U.getUser()));
+        }
----------------
jdoerfert wrote:
> fhahn wrote:
> > jdoerfert wrote:
> > > Why do we need to check these uses?
> > Unfortunately a `MemoryDef` does not necessarily have all may/must-alias defs that follow it as users. For example, I think we could have something like
> > 
> > ```
> >   %0 = MemoryDef (LiveOnEntry)
> >   %1 = MemoryPhi(%0,...)
> >   %2 = MemoryDef(%1,...) ; may-alias %0
> > ```
> >  
> > depending on what MemorySSA optimizations are applied, I think there could be similar examples with just MemoryDefs.
> I'm trying to wrap my head around this and it is probably me. I haven't worked with MSSA much.
> 
> So, `AccessesToCheck` starts with the defining access for each read location, so far so good. (correct me if I'm wrong at some point)
> If that access is outside the loop, done.
> If that access is inside and aliasing a location, done.
> If that access is inside and not aliasing a location, why do we look at the uses?
> I would understand if we look at the "operands":
> 
> ```
> Header:
> %1 = MemDef(%0,...) // clobbers %3
> %2 = MemDef(%1,...) // defining access of %3
> %3 = MemUse(%2,...) // in AccessedLocations
> ```
> 
> Though, I assume I simply do not understand MSSA in various ways.
> So, AccessesToCheck starts with the defining access for each read location, so far so good. (correct me if I'm wrong at some point)
> If that access is outside the loop, done.
> If that access is inside and aliasing a location, done.

Sounds good so far.

> If that access is inside and not aliasing a location, why do we look at the uses?

Because the uses of access `A` are the memory accesses that may read/write the memory location written by `A`, after `A`. For accesses that may access the same memory locations in a loop/cycle, there will be uses in `MemoryPhis`.

A concrete example is below. To visit all potential clobbers of `; MemoryUse(1) (  %lv = load i32, i32* %ptr, align 4)` , we have to visit all uses of `%1 = MemoryDef(4) (call void @clobber())`, their uses and so on.

The case where a clobber comes before the defining access in the header (as in your example) will be handled by this forward-scanning approach because there will be a `MemoryPhi` that's the defining access of `%1` in your example.

Does this make sense?

While looking at this again, I realized that queuing the defining access at line 692 could be overly pessimistic, if there are scenarios where it may alias a location in `AccessedLocations`, but its defining access is outside the loop. It would probably better to directly queue the uses of the defining access. But I am not sure if such a scenario can happen in practice.


```
define i32 @test(i32* %ptr, i32 %N) {
entry:
  br label %loop.header

loop.header:                                      ; preds = %loop.latch, %entry
; 4 = MemoryPhi({entry,liveOnEntry},{loop.latch,3})
  %iv = phi i32 [ 0, %entry ], [ %iv.next, %loop.latch ]
; 1 = MemoryDef(4)
  call void @clobber()
; MemoryUse(1)
  %lv = load i32, i32* %ptr, align 4
  %sc = icmp eq i32 %lv, 100
  br i1 %sc, label %noclobber, label %clobber

noclobber:                                        ; preds = %loop.header
  br label %loop.latch

clobber:                                          ; preds = %loop.header
; 2 = MemoryDef(1)
  call void @clobber()
  br label %loop.latch

loop.latch:                                       ; preds = %clobber, %noclobber
; 3 = MemoryPhi({noclobber,1},{clobber,2})
  %c = icmp ult i32 %iv, %N
  %iv.next = add i32 %iv, 1
  br i1 %c, label %loop.header, label %exit

exit:                                             ; preds = %loop.latch
  ret i32 10
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93764