[PATCH] D48627: [ImplicitNullChecks] Check for rewrite of register used in 'test' instruction

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:503
+  //
+  // gets incorrectly converted into:
+  //
----------------
This form of comment is useful for understanding the reasoning of the bug but after it is landed it will be difficult to read this comment..
why it is converted in this way?!

I would suggest to re-phrase it. Something like, to prevent the invalid transformation ... we must ensure that ...


================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:512
+  // jump that modify %rax.
+  unsigned cmpReg = MBP.LHS.getReg();
+
----------------
There is also the same assignment later:
const unsigned PointerReg.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:514
+
+  for (auto I = MBB.rbegin(); MBP.ConditionDef != &*I; ++I)
+    if (I->modifiesRegister(cmpReg, TRI))
----------------
I would suggest to move this check after "if (NotNullSucc->pred_size() != 1)" because the later seems simpler.



================
Comment at: lib/CodeGen/ImplicitNullChecks.cpp:514
+
+  for (auto I = MBB.rbegin(); MBP.ConditionDef != &*I; ++I)
+    if (I->modifiesRegister(cmpReg, TRI))
----------------
skatkov wrote:
> I would suggest to move this check after "if (NotNullSucc->pred_size() != 1)" because the later seems simpler.
> 
I would add an assert that MBP.ConditionDef is in MBB.


Repository:
  rL LLVM

https://reviews.llvm.org/D48627