[PATCH] D87108: [ImplicitNullCheck] Handle instructions that do not modify null behaviour of null checked reg

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Approach generally looks reasonable.  Please address nitpicks and then I plan to LGTM.



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1276
+  /// NullValueReg = rax.
+  virtual bool isNullBehaviourUnchanged(const MachineInstr *MI,
+                                        const Register NullValueReg,
----------------
minor name change: preservesZeroReg(...)

Null is simply zero in MI land, and we don't want to imply there's anything special about the pointer interpretation here.

I'd also suggest being really explicit in the comment that the register can become null.  This function only returns whether a zero is guaranteed to remain a zero.    


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3673
+  // X.
+  if (MI->getOpcode() == X86::SHR64ri || MI->getOpcode() == X86::SHR32ri ||
+      MI->getOpcode() == X86::SHL64ri || MI->getOpcode() == X86::SHR64ri)
----------------
Minor: Use a switch instead of a series of if clauses please.

Also, I believe you can handle all the rr variants here.

Also, please assert that operand 0 is a def and operand 1 is a use.  Mostly just to make it easier for someone with minimal context to understand form.


================
Comment at: llvm/test/CodeGen/X86/implicit-null-check.ll:595
+; Same as imp_null_check_load_shift_addr but shift is by 3 and this is now
+; converted into an LEA.
+; TODO: Can be converted into implicit null check
----------------
not an LEA - I think you meant complex addressing?  Or maybe the LEA gets folded late?

Regardless, the comment is out of sync with tested output.


================
Comment at: llvm/test/CodeGen/X86/implicit-null-check.ll:623
+   ret i64 %t
+}
 !0 = !{}
----------------
Please add a couple of counter examples.  Particularly:
%rax = shift %rdx, %rax  (i.e. wrong operand order)
%rax = add64ri %rax, 1 (i.e. a different instruction)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87108/new/

https://reviews.llvm.org/D87108