[PATCH] D82566: [CodeMoverUtils] Make specific analysis dependent checks optional

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:319
+  if (!DT)
+    return false;
 
----------------
asbirlea wrote:
> RithikSharma wrote:
> > Whitney wrote:
> > > RithikSharma wrote:
> > > > Whitney wrote:
> > > > > Actually if DI or PDT or DI, either one is nullptr, than this function never return true, so why not just check that in the beginning.
> > > > I understood your point. Say we don't have DT, PDT or DI then instead of returning false should we return true/false based on our best knowledge? If a client don't have DT, PDT and DI then it may be expecting at least some checks (which may or may not be of any real use)? Also, when we will add more analysis (example MSSA) then would it make more sense to return true/false based on whatever checks/analysis we have with us and not conservatively returning false?
> > > If DT, PDT, or DI is nullptr, we have to conservatively return false, we can only say is safe to move before if we are sure it is safe. If we have more analysis, for example MSSA, and it can tell if the instruction is for sure safe to move, then we can return true in that case. Does it make sense?
> > Yes it does, Updated.
> MSSA will always have DT available.
> I'm not clear why this is not keeping DT a reference.
Good point, we can keep DT as reference, all loop passes have access to DT.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:317
+  // Skip tests that need DT
+  if (!DT)
+    return false;
----------------
if we check here
```
if (!DT || !PDT || !DI)
  return false;
```
then we can save some compile time. As currently, there are no way that can return true, if any of them is nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82566