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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Seems like this needs to be rebased on top of D82290 <https://reviews.llvm.org/D82290>?



================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:319
+  if (!DT)
+    return false;
 
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/CodeMoverUtils.cpp:365
+  if (!(DT && PDT))
+    return true;
+
----------------
Whitney wrote:
> return false
Only check PDT. DT has been checked above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82566