Page MenuHomePhabricator

[ImplicitNullCheck] Fix the bug when dependent instruction accesses memory
ClosedPublic

Authored by skatkov on Aug 7 2017, 3:58 AM.

Details

Summary

It is possible that dependent instruction may access memory.
In this case we must reject optimization because the memory change will
be visible in null handler basic block. So we will execute an instruction which
we must not execute if check fails.

Diff Detail

Repository
rL LLVM

Event Timeline

skatkov created this revision.Aug 7 2017, 3:58 AM
sanjoy accepted this revision.Aug 7 2017, 9:36 AM

lgtm with minor comments

lib/CodeGen/ImplicitNullChecks.cpp
315 ↗(On Diff #109966)

IIUC this isn't optimal -- we can early return with AR_MayAlias when processing further would have given us AR_WillAliasEverything.

322 ↗(On Diff #109966)

Minor, but I'd be consistent here and either take both as refs or pointers.

343 ↗(On Diff #109966)

There are too many continues and returns here -- do you mind splitting out a mayAlias(MachineMemOperand *, MachineMemOperand *) helper?

369 ↗(On Diff #109966)

s/has value/have values/

470 ↗(On Diff #109966)

I vaguely remember that originally we did not allow the dependence MI to touch memory. Do you know when this changed (or am I misremembering)?

This revision is now accepted and ready to land.Aug 7 2017, 9:36 AM

Hi Sanjoy, thank you for the quick review and a good point about stores at all. I think this patch is incorrect and will upload other version soon.

lib/CodeGen/ImplicitNullChecks.cpp
470 ↗(On Diff #109966)

This is really good question and I came to it last night (before going to sleep) as well.
It seems it is incorrect. for DependenceMI we check that registers it defines are not used in null branch handling. actually the same we should do for memory change. So it means that if DependenceMI changes the memory we must ensure that memory is not used in null case handler because we did a change which should not happen if null happened. It is really difficult to do and it makes me thinking that we must just prohibit stores for DependenceMI.

Please note that we have already prohibit loads (line 428 this file). And we should do the same for store.

Actually I've discussed this today with Philip offline and he made a good point about DependenceMI. It should not do any visible side-effects to the branch handling null case. We already check the definition of registers. Also we do not allow different instructions like call. We should not allow stores (will upload a patch). I will also check whether we cover other cases if they exists.

skatkov updated this revision to Diff 110140.Aug 7 2017, 11:58 PM

Sanjoy, please take a look.

skatkov edited the summary of this revision. (Show Details)Aug 7 2017, 11:59 PM
sanjoy accepted this revision.Aug 8 2017, 6:11 PM

lgtm again

Thank you Sanjoy!
You asked about when we allowed stores. DependenceMI did not check for stores but it was hidden in canHandle. when I implemented INC based on stores and added alias analysis I removed the check from canHandle but did not add it to DependenceMI. so it was my fault.

This revision was automatically updated to reflect the committed changes.