[llvm-dev] Dereferenceable load semantics & LICM

[Chandler Carruth via llvm-dev]

On Mon, Apr 3, 2017 at 2:01 PM Piotr Padlewski <piotr.padlewski at gmail.com>
wrote:

> 2017-04-01 15:59 GMT+02:00 Piotr Padlewski <piotr.padlewski at gmail.com>:
>
>
>
> 2017-03-31 23:20 GMT+02:00 Sanjoy Das <sanjoy at playingwithpointers.com>:
>
> Hi Piotr,
>
> On March 31, 2017 at 1:07:12 PM, Piotr Padlewski
> (piotr.padlewski at gmail.com) wrote:
> > [snip]
> > Do I understand it correctly, that it is legal to do the hoist because
> all
> > of the instructions above %vtable does not throw?
>
> Yes, I think you're right.  HeaderMayThrow is a conservative
> approximation, and the conservativeness is biting us here.
>
> > Are there any plans to fix it in the future? The fix doesn't seem hard to
>
> Not to my knowledge.
>
> > write and I can do it, but I am not sure if it won't be too expensive.
>
> Maybe we can do it (relatively) cheaply:
>
>  - Remember the first throwing instruction in the header, instead of a
>    boolean, in LoopSafetyInfo
>
>  - In hoistRegion, remember if you've seen the first throwing
>    instruction yet
>
>  - Pass the above as a boolean parameter to isGuaranteedToExecute, and
>    instead of
>      if (Inst.getParent() == CurLoop->getHeader())
>        return !SafetyInfo->HeaderMayThrow;
>    do something like
>      if (Inst.getParent() == CurLoop->getHeader())
>        return IsBeforeThrowingInst;
>
> -- Sanjoy
>
> I was thinking about something very similar and it seems to be a good
> approach to me because it has much lower complexity.
>
> In the review Sanjoy correctly spoted that I am not discarding
> invariant.group metadata when hoisting, which is incorrect in general.
> This generates a big problem to devirtualization, because discarding
> metadata when hoisting vtable load might be worse than not hoisting when
> there might be another load/store with !invariant.group dominating it
> (e.g. after inlining).
>
> I don't see any way to model it right now in LLVM, but I see a one simple
> solution:
>  add extra information to the metadata nodes indicating that this property
> is non-local:
>
> %0 = load... %p, !invariant.group !1, !dereferenceable !2
> %1 = load ... %0, !invariant.load !0, !dereferenceable !2
>
> !0 = !{!"GlobalProperty"}
> !1 = !{!"MyType", !"GlobalProperty"}
> !2 = !{i64 8, !"GlobalProperty}
>
> With that we would strip only the metadata not containing this information.
>
> For devirtualization it would make sense with invariant.load,
> invariant.group and dereferenceable.
>
> What do you think about this idea?
>
> Do you have any comments? I would like to know if the idea does even make
> sense, so I won't start writing prototype that will be throwed to thrash
>

I don't really see why you need a separate property....

It seems plausible that rather than having the hoisting logic handle
invariant metadata generically (by stripping it when hoisting) it could
instead be aware of invariant metadata and try to avoid hoisting the load.

But this will end up being yet another way in which enabling invariants
obstructs normal optimizations to get devirtualization. I'm increasingly
worried that the cost is going to outweigh the gain.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170404/5e7ad414/attachment.html>