This is an archive of the discontinued LLVM Phabricator instance.

[CodeMoverUtils] Make specific analysis dependent checks optional
ClosedPublic

Authored by RithikSharma on Jun 25 2020, 8:47 AM.

Details

Summary

This patch makes code motion checks optional which are dependent on specific analysis example, dominator tree, post dominator tree and dependence info. The aim is to make the adoption of CodeMoverUtils easier for clients that don't use analysis which were strictly required by CodeMoverUtils. This will also help in diversifying code motion checks using other analysis example MSSA.

Diff Detail

Event Timeline

RithikSharma created this revision.Jun 25 2020, 8:47 AM
Whitney requested changes to this revision.Jun 25 2020, 9:13 AM
Whitney added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
514

where is no need to change LoopFuse, just pass the address when calling the APIs.

llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
102

you can a segfault here when DT is nullptr, making analyses optional means you have to add handle when they are nullptr, and add alternative ways to do what the function is intended to do.

318

so as long as DT is nullptr, instruction is safe to move before InsertPoint? can return false here instead of true.

This revision now requires changes to proceed.Jun 25 2020, 9:13 AM
RithikSharma marked 3 inline comments as done.Jun 26 2020, 5:49 AM
RithikSharma added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
102

Acknowledged but right now ControlConditions or its member functions are not called when we don't have DT or PDT. ControlConditions don't know that DT and PDT are optional, isn't it be handled by ControlCondition's clients?

318

Acknowledged and updated

Whitney added inline comments.Jun 26 2020, 12:52 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
102

in that case change the parameters back to references for ControlConditions

RithikSharma marked an inline comment as done.
RithikSharma added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
102

Thanks, updated.

Whitney added inline comments.Jun 28 2020, 8:22 PM
llvm/include/llvm/Transforms/Utils/CodeMoverUtils.h
30

what's the reason why some arguments have nullptr as default value, and some doesn't?

llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
226

Is there an alternative way to check control flow equivalent without DT and PDT? If not, change it back to references.

231

here you are dereferencing a pointer that can be a nullptr.

318

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.

364

return false

372

return false

llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
34

I prefer not to change any of the parameters from references to pointers, just change the call site.

RithikSharma marked an inline comment as done.Jun 29 2020, 6:51 AM
RithikSharma added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
318

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?

Whitney added inline comments.Jun 29 2020, 7:17 AM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
318

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?

RithikSharma marked an inline comment as done.
RithikSharma added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
318

Yes it does, Updated.

RithikSharma marked 2 inline comments as done.Jun 29 2020, 7:42 AM

Seems like this needs to be rebased on top of D82290?

llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
318

MSSA will always have DT available.
I'm not clear why this is not keeping DT a reference.

364

Only check PDT. DT has been checked above.

Whitney added inline comments.Jun 29 2020, 2:15 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
317

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.

318

Good point, we can keep DT as reference, all loop passes have access to DT.

Many thanks, I have updated the diff.

Whitney accepted this revision.Jul 6 2020, 10:58 AM
This revision is now accepted and ready to land.Jul 6 2020, 10:58 AM
This revision was automatically updated to reflect the committed changes.