[PATCH] D36392: [ImplicitNullCheck] Fix the bug when dependent instruction accesses memory

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

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.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:470
+  // Verify that DependenceMI does not alias with any instructions seen so far.
+  if (DependenceMI->mayLoadOrStore())
+    if (areMemoryOpsAliased(*DependenceMI, { InstsSeenSoFar.begin(),
----------------
sanjoy wrote:
> 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 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.


https://reviews.llvm.org/D36392