[PATCH] This patch introduces MemorySSA, a virtual SSA form for memory.Details on what it looks like are in MemorySSA.h
Philip Reames
listmail at philipreames.com
Mon Apr 13 13:46:32 PDT 2015
On 04/13/2015 12:54 PM, Daniel Berlin wrote:
> On Mon, Apr 13, 2015 at 10:33 AM, Philip Reames
> <listmail at philipreames.com> wrote:
>
>> I would also strongly prefer to see more test cases included. If nothing else, having a set of small examples for the semi obvious cases would make
>> the usage of memory ssa much easier to understand.
> I am happy to generate more testcases. I originally had some unit
> tests that verified a bunch more (including the individual values),
> then moved them to printing tests because that is apparently the
> preference where possible.
> In any case, I will add more tests for common/not so common cases i
> have hit to make it obvious what is supposed to happen.
>
> (I am also going to add API unittests for the the updating API anyway,
> since it seems more difficult to do those as printing tests, since you
> want to be able to verify immediate uses and such).
Sounds good.
>
>
>> ================
>> Comment at: include/llvm/Analysis/AliasAnalysis.h:508
>> @@ +507,3 @@
>> + /// memory used by a given call
>> + virtual bool instructionClobbersCall(Instruction *I,
>> + ImmutableCallSite Call);
>> ----------------
>> This seems like a subset of the getModRefInfo interface. Is there a strong reason it needs to be phrased differently?
> Do you mean you think this should be getModRefInfo(Instruction *,
> ImmutableCallSite) (and return ModRefResult)
>
> If so, SGTM, i'll update it.
That was exactly what I meant.
>
>> One possible interesting case, what happens if you have a partially overlapping memcpy? Is that a single user or two uses?
> So, because of the way it is structured, it is a single user.
> The same with phi's. If you appear in a phi node multiple time, you
> will still only get one use.
>
> To give you some background, I looked at how Use.h worked, and it
> seemed really tricky to get that working as a first pass, so i punted
> on it and implemented Users.
>
> We may want uses at some point, particularly because it makes
> determining the path to the phi node easier for downwards-exposed
> info.
> (IE If you are a store, and ask for the users, given you may occur
> multiple times in the same phi node, unless your store is in an
> immediate predecessor, it seems tricky to determine which phi node
> operand matches up with the store without walking the CFG)
Users seems entirely reasonable. We just need to make sure the API
naming is consistent with that. I am not asking you to switch to uses,
just to be clear about which you're exposing.
>
>
>> ================
>> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:132
>> @@ +131,3 @@
>> +protected:
>> + friend class MemorySSA;
>> + friend class MemoryUse;
>> ----------------
>> Why do these need to be friends? Shouldn't protected access be enough? I *really* dislike having friend declarations here unless they're absolutely necessary.
> Otherwise, you get this:
>
> ./lib/Transforms/Utils/MemorySSA.cpp:770:13: error: 'getID' is a
> protected member of 'llvm::MemoryAccess'
> if (MA->getID() != 0)
> ^
> ../include/llvm/Transforms/Utils/MemorySSA.h:150:24: note: can only
> access this member on an object of type 'llvm::MemoryPhi'
> virtual unsigned int getID() const { return 0; }
>
> (and others), because you can't access them from the other classes.
>
> So, for example, when the memoryphi tries to print operands, it gets
> this error because they are not MemoryPhi's, so protected does not
> give memoryphi class access to call it.
friend is a *very* coarse grained mechanism though. I'd almost prefer
to see these few methods on the public interface with a clear comment.
I'll defer to your choice here, but my previous experience with friend
declarations has not been pleasant.
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:227
> @@ +226,3 @@
> +/// \brief Represents a read-write access to memory, whether it is real, or a
> +/// clobber.
> +///
> ----------------
> Define clobber. Preferrably using mayalias terminology. References to AA docs are fine.
> I updated this to be exact (and the same for MemoryUse).
> MemoryUse now says:
>
> /// In particular, the set of Instructions that will be represented by
> /// MemoryUse's is exactly the set of Instructions for which
> /// AliasAnalysis::getModRefInfo returns "Ref".
>
> MemoryDef now says:
> /// In particular, the set of Instructions that will be represented by
> /// MemoryDef's is exactly the set of Instructions for which
> /// AliasAnalysis::getModRefInfo returns "Mod" or "ModRef".
Ok
>
>
>> ================
>> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:260
>> @@ +259,3 @@
>> +/// These have the same semantic as regular phi nodes, with the exception that
>> +/// only one phi will ever exist in a given basic block.
>> +
>> ----------------
>> Is this still true with the mayalias changes?
> Yes. Ensuring that is why we do not disambiguate the RHS of MemoryDef's
> (A disambiguated use will never cause the reaching *definition* to
> change, because they are uses.
> Disambiguated defs may cause multiple reaching definitions, and thus
> we disallow them.
> )
I couldn't parse your response here. Could restate this in different words?
>
>
>> If so, how does that work? Respond as a comment in code please.
>>
> Done.
>
> (It now says:
> /// These have the same semantic as regular phi nodes, with the exception that
> /// only one phi will ever exist in a given basic block.
> /// Guaranteeing one phi per block means guaranteeing there is only ever one
> /// valid reaching MemoryDef/MemoryPHI along each path to the phi node.
> /// This is ensured by not allowing disambiguation of the RHS of a MemoryDef or
> /// a MemoryPHI's operands.
> /// That is, given
> /// if (a) {
> /// store %a
> /// store %b
> /// }
> /// it *must* be transformed into
> /// if (a) {
> /// 1 = MemoryDef(liveOnEntry)
> /// store %a
> /// 2 = MemoryDef(1)
> /// store %b
> /// }
> /// and *not*
> /// if (a) {
> /// 1 = MemoryDef(liveOnEntry)
> /// store %a
> /// 2 = MemoryDef(liveOnEntry)
> /// store %b
> /// }
> /// even if the two stores do not conflict otherwise, both 1 and 2 reach the
> /// end of the branch, and if there are not two phi nodes, one will be
> /// disconnected completely from the SSA graph below that point.
> /// Because MemoryUse's do not generate new definitions, they do not
> have this issue.
Ok.
>
>> ================
>> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:407
>> @@ +406,3 @@
>> + /// \brief Remove a MemoryAccess from MemorySSA, including updating all
>> + // definitions and uses.
>> + void removeMemoryAccess(MemoryAccess *);
>> ----------------
>> When should this (and the next few routines) be used?
> Whenever you change memory accesses.
> I'll update the documentation a bit for each of these.
Thanks.
> Currently the only user is the new MergedLoadStoreMotion and NewGVN.
And when I edit one of those, I'd really like to know what the intended
uses are. :)
>
>> ================
>> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:553
>> @@ +552,3 @@
>> + /// Note that this will return a single access, and it must dominate the
>> + /// Instruction, so if an argument of a MemoryPhi node clobbers thenstruction,
>> + /// it will return the memoryphi node, *not* the argument.
>> ----------------
>> This comment is unclear?
> This portion now reads:
>
> ///
> /// Note that this will return a single access, and it must dominate the
> /// Instruction, so if an operand of a MemoryPhi Mod's the instruction,
> /// this will return the MemoryPhi, not the operand. This means that
> /// given:
> /// if (a) {
> /// 1 = MemoryDef(liveOnEntry)
> /// store %a
> /// } else {
> /// 2 = MemoryDef(liveOnEntry)
> /// store %b
> /// }
> /// 3 = MemoryPhi(2, 1)
> /// MemoryUse(3)
> /// load %a
> ///
> /// calling this API on load(%a) will return the MemoryPhi, not the MemoryDef
> /// in the if (a) branch.
Much clearer. Thanks.
>
>
>> ================
>> Comment at: lib/Analysis/AliasAnalysis.cpp:351
>> @@ -332,3 +350,3 @@
>> // or write the specified memory.
>> - if (!alias(getLocation(L), Loc))
>> + if (Loc.Ptr && !alias(getLocation(L), Loc))
>> return NoModRef;
>> ----------------
>> These *need* to submitted separately with test cases if possible.
> So, you can't make it crash in the current callers, but you can with MemorySSA.
>
> In this case, our goal is just to determine whether this is an
> instruction that reads/writes memory, without regard to a specific
> location.
To rephrase this, a Location with a null Ptr describes all possible
locations? (Subject to the AA metadata restrictions?) If so, can you
update the AA docs for location to make this explicit?
>
> But, if you call getModRefInfo with an empty Loc.Ptr, it will crash on
> the instructions I updated.
> How do you get an empty Loc.Ptr?
>
> By passing a default constructed AliasAnalysis::Location to them.
>
> This (passing default constructed AliasAnalysis::Location) does not
> happen in the other caller (MemoryDependenceAnalysis), because it
> specifically avoids it through other tricks.
Sounds like we should be updating MDA after your change. :)
>
> I used to handle this in MemorySSA by filling in a fake Loc.Ptr. Hal
> suggested I just update AliasAnalysis to not crash instead, which will
> let use remove the need for tricks at all (after all, nothing in the
> API makes it invalid to pass default constructed locations to these
> functions))
I agree with Hal.
>
> In any case, i will submit it separately, but note that every testcase
> that uses memoryssa would trigger this bug otherwise (since memoryssa
> will call getModRefInfo on every instructions, and will crash on the
> load/stores as a result)
Can I ask you to write a C++ API test for this? This should not
regress, no matter what happens to MemorySSA.
>
>
>> http://reviews.llvm.org/D7864
>>
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
More information about the llvm-commits
mailing list