[PATCH] D29400: [ImplicitNullCheck] Extend Implicit Null Check scope by using stores

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm, will check this in for you now after running clang-format

(Please ask Chris Lattner for commit access -- http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access  :) )



================
Comment at: include/llvm/CodeGen/FaultMaps.h:29
 public:
-  enum FaultKind { FaultingLoad = 1, FaultKindMax };
+  enum FaultKind { FaultingLoad = 1, FaultingStore, FaultKindMax };
 
----------------
skatkov wrote:
> sanjoy wrote:
> > I found the earlier classification system better, with a third `FaultingLoadStore` category.  Right now we don't have a place to put `addq (%rax), %rbx` type instructions, which both load and store.
> I considered that for that kind of instruction we first read and failed on this :) But ok, I will return it back.
Okay, if all of the load-and-store instructions load before storing then I'm fine changing it back in a later change (but let's not waste another review cycle on it right now).


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:308
+
+  SeenLoad = SeenLoad || MI.mayLoad();
+
----------------
skatkov wrote:
> sanjoy wrote:
> > How about `SeenLoad |= MI.mayLoad()`?
> Sanjoy, as I understand in my case mayLoad will be einvoked only if SeenLoad == false, in |= form mayLoad will be invoked always and there is no ||= operator.
I'd normally assume that the host compiler would be smart enough to optimize `SeenLoad || MI.MayLoad()` into `SeenLoad | MI.MayLoad()`, but your decision is fine by me.



https://reviews.llvm.org/D29400