Page MenuHomePhabricator

[InlineCost] Adjust inlining cost for implicit null checks
AbandonedPublic

Authored by reames on Sep 22 2015, 8:48 PM.

Details

Reviewers
chenli
Summary

Implicit null check is a mechanism that fold null pointer checks into nearby memory operations. It simply means that for null pointer comparisons, the execution will unconditionally fall through the non-null branch and let the load of the pointer handle the false case (it can use store instruction as well but LLVM currently only supports load instruction). For more information, please refer http://llvm.org/docs/FaultMaps.html

Because implicit null checks act as unconditional branches, their inlining cost should be free.

Diff Detail

Event Timeline

chenli updated this revision to Diff 35460.Sep 22 2015, 8:48 PM
chenli retitled this revision from to [InlineCost] Adjust inlining cost for implicit null checks.
chenli updated this object.
chenli added reviewers: reames, chandlerc.
chenli added a subscriber: llvm-commits.
reames added inline comments.Sep 23 2015, 12:19 PM
lib/Analysis/InlineCost.cpp
622

It looks like this might be a partial diff? I don't see any changes in visitBranchInst?

chenli added inline comments.Sep 23 2015, 12:22 PM
lib/Analysis/InlineCost.cpp
622

Apparently I uploaded the wrong diff. Will update soon.

chenli updated this revision to Diff 35548.Sep 23 2015, 1:31 PM

Update patch and test case.

reames edited edge metadata.Sep 23 2015, 5:36 PM

A a meta level, I feel this this patch is approaching the icmp part the wrong way. Rather than trying to determine if a particular comparison is a implicit null check, we should check to see if a particular *branch* is likely implicit and if so, discount the cost of both the branch and the icmp. I'm also not sure it's far to entirely discount the cost of both. We can't from the IR guarantee that the check will actually be made implicit.

Also, can you add a bit of context to your description of the change? I know what an implicit null check is, but not everyone will.

Chandler, since you're sure to have strong opinions, what do you feel is the right approach for framing this?

A a meta level, I feel this this patch is approaching the icmp part the wrong way. Rather than trying to determine if a particular comparison is a implicit null check, we should check to see if a particular *branch* is likely implicit and if so, discount the cost of both the branch and the icmp. I'm also not sure it's far to entirely discount the cost of both. We can't from the IR guarantee that the check will actually be made implicit.

I dont feel like the icmp part should be handled in visitBranch. My reasoning is that the icmp might have other user as well. You should not treat it as free if only one of the users is implicit null check. You need to know all its users to decide whether it can be free. That's why I put the check in visitCmp instead of visitBranch.

I agree that it might not be proper to entirely discount the cost as there're chances the lowering part cannot make it implicit. I'd very like to hear more opinions on this.

Also, can you add a bit of context to your description of the change? I know what an implicit null check is, but not everyone will.

Thanks for mentioning. I will update the description.

Chandler, since you're sure to have strong opinions, what do you feel is the right approach for framing this?

chenli updated this object.Sep 25 2015, 8:38 PM
chenli edited edge metadata.
chenli updated this object.Sep 25 2015, 8:40 PM
sanjoy added a subscriber: sanjoy.Sep 27 2015, 3:12 PM

I've mentioned this to Chen in person, but I'll repeat what I said:

I'm not sure I understand what the "cost" in this context is supposed
to model. If it is supposed to model code size then this change makes
sense completely. If it is supposed to model *compile time* then this
change is not in the right direction -- the implicit null checks stay
explicit throughout optimization, so they will extract the compile
time cost of a "normal" compare and branch.

reames commandeered this revision.May 18 2017, 9:22 AM
reames edited reviewers, added: chenli; removed: reames.
reames abandoned this revision.May 18 2017, 9:22 AM