Page MenuHomePhabricator

[Attributor] AAFunctionReachability, Instruction reachability.
AcceptedPublic

Authored by kuter on Jul 23 2021, 3:54 PM.

Details

Summary

This patch implement instruction reachability for AAFunctionReachability
attribute. It is used to tell if a certain instruction can reach a function
transitively.

Diff Detail

Event Timeline

kuter created this revision.Jul 23 2021, 3:54 PM
kuter requested review of this revision.Jul 23 2021, 3:54 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter updated this revision to Diff 361374.Jul 23 2021, 4:09 PM

Don't ignore the return value of checkForAllCallLikeInstructions

Add doxygen docu to the functions to describe what they do.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9684

Don't accumulate pointers to containers but content instead.

9780–9781

|=, also elsewhere
no braces.

jdoerfert added inline comments.Jul 25 2021, 9:41 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
4467

Spell inst out in the function name, pass function as reference, (or is a nullptr ok?)

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9727

I don't get the backwards stuff.

kuter added inline comments.Jul 26 2021, 7:32 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9684

I tried using references here. But I couldn't pass SmallVector<const AACallEdges &> as a ArrayRef.

9780–9781

Oh great. I am happy to hear that someone finally added that operator.

jdoerfert added inline comments.Jul 26 2021, 8:28 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9700

I think this is a problem if you have invokes.
Dies the instCanReach include the instruction or not? If it's the latter we can just pass the call site.

9716

don't ignore the return value

9724

no braces.

llvm/unittests/Transforms/IPO/AttributorTest.cpp
207

can we have a negative instcanreach test?

kuter updated this revision to Diff 361930.Jul 27 2021, 12:38 AM

Use the new |= operator for change tracking.

kuter updated this revision to Diff 362568.Jul 28 2021, 3:37 PM
  • Explicitly handle InvokeInst.
  • Add a option to note use backwards reachability.
  • Don't use backwards reachability for transative queries (fixes the fixme)
  • Address misc review.
kuter retitled this revision from [WIP][Attributor] AAFunctionReachability, Instruction reachability. to [Attributor] AAFunctionReachability, Instruction reachability..Aug 22 2021, 4:28 PM
jdoerfert added inline comments.Aug 23 2021, 8:54 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9614

Is this the right way? I would have thought we ignore CB if CBInst is not reachable from Inst, not the other way around. We need a test I suppose.

9640–9641

At least add a TODO. To fix it proper you can look at the two successors blocks and the first non-debug instruction in them.

9648–9650

Why no range loop?

kuter added inline comments.Aug 23 2021, 8:42 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9648–9650

markReachable call below erases Fn from the set we are iterating right now.
I increment the iterator before processing Fn in case Fn gets removed from the Unreachable set.

It would be invalid to increment the iterator if Fn was already deleted right ? I think we have to do that before.

jdoerfert added inline comments.Aug 24 2021, 7:50 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9648–9650

I see. We have a special iterator + range loop for that, maybe you want to look at make_early_inc_range and it's uses.

kuter added inline comments.Sat, Sep 11, 11:36 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9614

We are doing an early return if the CB is not assumed reachable. If not we add it into the Result Vector. We later use the result of this function for QueryResolver::update or QueryResolver::isReachable.

So the return true here doesn't mean anything.

jdoerfert added inline comments.Sat, Sep 11, 3:23 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9614

I was not clear. My concern is that we check if CBInst may reach Inst while we want to know if Inst may reach CBInst, no?
So if (!Reachability.isAssumedReachable(A, Inst, CBInst)) instead?

kuter updated this revision to Diff 372568.Tue, Sep 14, 2:42 PM
kuter marked 7 inline comments as done.

Address review.

jdoerfert added inline comments.Tue, Sep 14, 5:18 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9656–9661
9735

Reachable is not updated if CanReachUnknownCallee is set true. We also never check for a cached value here.

9752

One of these caused a copy and only the copy QuerySet got updated causing it never to find a fixpoint.

9769

One of these caused a copy and only the copy QuerySet got updated causing it never to find a fixpoint.

9779

One of these caused a copy and only the copy QuerySet got updated causing it never to find a fixpoint.

kuter updated this revision to Diff 372618.Tue, Sep 14, 7:30 PM
kuter marked 5 inline comments as done.

Address review.

jdoerfert accepted this revision.Fri, Sep 17, 11:36 AM

Two comments below, otherwise LG.

As we add users we will need to extend/adjust some thing but conceptually this should be OK.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
9617

Replace static cast with cast<CallBase> and it is not Inst but CBInst you want to cast here.

9808

Documentation.

This revision is now accepted and ready to land.Fri, Sep 17, 11:36 AM

Testing it more I see this crashing and the backwards stuff not working properly. Probably easiest for me to put a fix up here.