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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
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)
----------------
reames wrote:
> 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.
I tried writing up the register variant and got compile errors that such opcodes don't exist (i.e. SHL64rr etc..). Checked the intel guide and indeed we only support ri and mi variants, i.e. the MI.getOperand(2) is guaranteed to be an immediate. 


================
Comment at: llvm/test/CodeGen/X86/implicit-null-check.ll:623
+   ret i64 %t
+}
 !0 = !{}
----------------
reames wrote:
> 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)
> 
wrong operand order for shift doesn't work (see above comment about rr variant). will add the different instruction case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87108