[PATCH] D25542: GVN-hoist: avoid calling MemoryLocation::get() on a call (PR30499)

[Daniel Berlin via llvm-commits]

(as an aside, Hal, i'd love your opinion on whether MemoryLocation should
really be something that handles calls as well.  Or, alternatively, at
least getModRefInfo uses something that does without people having to know
about it.

If we used something like MemoryLocOrCall [obviously this is just a simple
union class]), we could  simplify parts of the AA API.  I also wouldn't
need it to hack around things in the AA API's getModRefInfo.

Right now we have this artificial distinction where we have things that use
pointers get MemoryLocations, other things get ImmutableCallSites. This
kinda makes sense for alias() (though more advanced aa's are just
implementing that in terms of mod ref info they have created).

However, the current AA API operates on both *anyway*, and the overloads
you get are confusing.

IE we have:

00333   ModRefInfo
<http://llvm.org/docs/doxygen/html/namespacellvm.html#a3994f46b116d674bc6d490897b9f60b9>
getModRefInfo <http://llvm.org/docs/doxygen/html/classllvm_1_1AAResults.html#ad5fb719206e3111dd6d4216d954395d5>(ImmutableCallSite
<http://llvm.org/docs/doxygen/html/classllvm_1_1ImmutableCallSite.html>
CS <http://llvm.org/docs/doxygen/html/namespacellvm.html#a488b094cb42981f433eaefd848483445a4f8b402109de7f745e3e37bb2dee4318>,
const MemoryLocation
<http://llvm.org/docs/doxygen/html/classllvm_1_1MemoryLocation.html>
&Loc);

and

00751 <http://llvm.org/docs/doxygen/html/classllvm_1_1AAResultBase_1_1AAResultsProxy.html#a537129572d9230c146f0533d1286cce2>
    ModRefInfo <http://llvm.org/docs/doxygen/html/namespacellvm.html#a3994f46b116d674bc6d490897b9f60b9>
getModRefInfo <http://llvm.org/docs/doxygen/html/classllvm_1_1AAResultBase_1_1AAResultsProxy.html#a537129572d9230c146f0533d1286cce2>(ImmutableCallSite
<http://llvm.org/docs/doxygen/html/classllvm_1_1ImmutableCallSite.html>
CS1, ImmutableCallSite
<http://llvm.org/docs/doxygen/html/classllvm_1_1ImmutableCallSite.html>
CS2);


etc


You pretty much can't use getModRefInfo right now unless you check whether
the instructions you are trying to get mod ref info about are  0, 1, or 2
calls.  Because calls don't have memorylocs, so you can't just always call
::get on them.
You then have to call the right overloads yourself.
That seems wrong.
(I'll ignore the fact that you also have to check whether you are holding a
fence inst, since you can't get memorylocations for those either, and ::get
will just crash.  )

Right now the number of overloads is also ... high, and while we fixed the
last immediate problem i remember (where random things would get overloaded
to ImmutableCallSite versions when they weren't calls), staring at the
number of getModRefInfo overloads, i'm not sure i believe the way we have
it structured makes sense.



On Wed, Oct 12, 2016 at 8:26 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

> You can get rid of UseInst too, MemoryLocOrCall will take a MemoryUse
> directly :)
>
>
> On Wed, Oct 12, 2016 at 8:20 PM, Sebastian Pop <sebpop at gmail.com> wrote:
>
>> On Wed, Oct 12, 2016 at 10:05 PM, Daniel Berlin <dberlin at dberlin.org>
>> wrote:
>> > There is actually a much simpler fix you sholdl use.
>> >
>> > return instructionClobbersQuery(MD, MU MemoryLocOrCall(MU), AA);
>> >
>> > You need to move that version of instructionClobbersQuery up, but it was
>> > built to always do the right thing.
>> >
>> > Also note it will properly handle a case you are not: Fences do not have
>> > memorylocations, and calling get on them will also crash.
>> >
>>
>> Right. I will commit the attached patch after testing.
>>
>> Thanks,
>> Sebastian
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161012/a702713a/attachment.html>