Page MenuHomePhabricator

[CodeMoverUtils] Add optional data dependence checks using Alias Analysis
Needs ReviewPublic

Authored by RithikSharma on Sat, Jul 25, 11:53 AM.

Details

Summary

isSafeToMoveBefore uses Dependence Info to check for flow/anti/output dependence, this patch adds alternative checks using Alias Analysis.

Diff Detail

Event Timeline

RithikSharma created this revision.Sat, Jul 25, 11:53 AM
Whitney added inline comments.Sat, Jul 25, 12:44 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
230

Please add a description for each isDependenceSafe(). And declare them as static.

231

SmallPtrSet<Instruction *, 10> -> SmallPtrSetImpl<Instruction *>

Same for the other isDependenceSafe().

258

check this at line 251.

if (AA.isNoAlias(MemLoc, DestMemLoc))
  return false;
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
46

Does DataLayout DL(""); work?

RithikSharma marked 5 inline comments as done.
RithikSharma added inline comments.
llvm/unittests/Transforms/Utils/CodeMoverUtilsTest.cpp
46

Thanks, yes it works.

Whitney added inline comments.Sun, Jul 26, 2:45 AM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
230

Instructions -> instructions

236

return (DepResult && (DepResult->isOutput() || DepResult->isFlow() || DepResult->isAnti()));

258

check isNoAlias before querying getModRefInfo

269

using dependence info or alias analysis

RithikSharma marked an inline comment as done.
RithikSharma marked 4 inline comments as done.

Acknowledged, updated.

Whitney accepted this revision.Sun, Jul 26, 3:20 AM
This revision is now accepted and ready to land.Sun, Jul 26, 3:20 AM
bmahjour requested changes to this revision.Mon, Jul 27, 7:48 AM
bmahjour added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
252

MemoryLocation::get returns "none" if Inst is a call. Please add special handling for calls and make sure your tests cover it.

260

The ref bit could be on for mod-ref cases. Please also check to make sure they are not mod.

This revision now requires changes to proceed.Mon, Jul 27, 7:48 AM
RithikSharma marked an inline comment as done.Tue, Jul 28, 7:24 AM
RithikSharma added inline comments.
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
252

Test case pending.

bmahjour added inline comments.Tue, Jul 28, 2:32 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
264

Inst (from the list of instructions being checked) may also be a call.

bmahjour added inline comments.Tue, Jul 28, 2:36 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
249

Not all aliasing implications are visible through the arguments. AAResults contains a number of overloads specifically designed to handle function calls. Did you look into using the following instead?

ModRefInfo getModRefInfo(const CallBase *Call, const MemoryLocation &Loc);
ModRefInfo getModRefInfo(Instruction *I, const CallBase *Call);
ModRefInfo getModRefInfo(const CallBase *Call1, const CallBase *Call2);
fhahn added a comment.Tue, Jul 28, 2:54 PM

Are there plans on moving some transforms to use this with AA? I think it would be good to have a concrete plan to make use of this, otherwise I am worried that the code will miss out on large real-world test coverage.

RithikSharma marked 2 inline comments as done.Tue, Jul 28, 11:31 PM
RithikSharma marked an inline comment as not done.Wed, Jul 29, 9:40 AM

Are there plans on moving some transforms to use this with AA? I think it would be good to have a concrete plan to make use of this, otherwise I am worried that the code will miss out on large real-world test coverage.

This was inspired from LICM, I'm trying to test this part by manually plugging this into LICM.

RithikSharma marked an inline comment as done.
hiraditya added inline comments.Sat, Aug 1, 10:10 PM
llvm/lib/Transforms/Utils/CodeMoverUtils.cpp
248

Any reason we have '10' instead of a value which is power of 2?

260

Once IsSafe is false. should we just return and avoid subsequent checks?

298

else is not needed,

387–388

comment needs update.

458

Are we planning to make use of AA here?

RithikSharma marked 4 inline comments as done.Mon, Aug 3, 9:29 AM

Many thanks for you reviews @hiraditya , acknowledged and updated.