[PATCH] D83708: Remove TwoAddressInstructinoPass::sink3AddrInstruction.

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked an inline comment as done.
nickdesaulniers added a comment.

LGTM; tests pass and kernel panic is fixed.  Just minor nits on things we might be able to simplify further in the test.



================
Comment at: llvm/test/CodeGen/X86/callbr-asm-sink.ll:8
+
+%struct1 = type { i8*, i8*, i8*, i32 }
+
----------------
Looks like we only access the third element of this struct in the GEP, IIUC the GEP correctly?


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-sink.ll:24
+  %2 = getelementptr inbounds %struct1, %struct1* %0, i64 0, i32 3
+  callbr void asm sideeffect "# $0 $1 $2", "*m,er,X,~{memory},~{dirflag},~{fpsr},~{flags}"(i32* %2, i32 1, i8* blockaddress(@klist_dec_and_del, %3))
+          to label %6 [label %3]
----------------
I think you can drop the constant `i32 1` from the `callbr` and its `er,` flags.


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-sink.ll:34
+6:
+  ret i32 undef
+}
----------------
`ret void`?


================
Comment at: llvm/test/CodeGen/X86/reverse_branches.ll:69
+; CHECK-NEXT:    cmpq %rax, %rbx
+; CHECK-NEXT:    movq %r13, %rdi
 ; CHECK-NEXT:    je LBB0_3
----------------
This move into `%rdi` after the call to `_memchr` is curious to me.  Doesn't the x86_64 ABI that darwin uses use `%rdi` as the first parameter to a function call?  Before hand, it looks like we're setting up the registers for the call as part of the calling convention.  After, it looks like we're maybe misplacing the `mov` into `%rdi`?

If we jump to `LBB0_5`, we kill `%rdi` immediately.

Ah, `LBB0_1` will have loaded `%r15` into `%rdi` for `LBB0_3`.

Ok, I think this is correct, but would appreciate others to pay careful attention to this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83708