[PATCH] D21325: [X86] Fix tailcall return address clobber bug

[Quentin Colombet via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

Hi Magnus,

The refactoring of findDeadCallerSavedReg looks good, though at this point don't know if it is necessary.
The merge SP update part seems like an optimization to me, and it feels wrong to have to resort on an optimization to generate correct code.

How was this working previously?
Or put different why is the merge SP update necessary to fix the issue?

Cheers,
-Quentin


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:200
@@ +199,3 @@
+    return 0;
+  } else {
+    SmallSet<uint16_t, 8> Uses = getReturnInstructionUses(MBBI, TRI);
----------------
remove else {
Per llvm coding style.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:200
@@ +199,3 @@
+    return 0;
+  } else {
+    SmallSet<uint16_t, 8> Uses = getReturnInstructionUses(MBBI, TRI);
----------------
qcolombet wrote:
> remove else {
> Per llvm coding style.
No need for brackets for one line block, per llvm coding style.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:432
@@ +431,3 @@
+      Offset += (Opc == X86::POP64r ? 8 : 4);
+      MBB.erase(PI);
+      if (MBBI == MBB.begin())
----------------
Add comments on what you are doing here.
For instance, something along the line of: this is fine to drop pop instructions when their results in not used by the return instruction, i.e., the values are not live out.

Explain why we need to make a special case for RIP. That is not clear, at least, to me. I would have expected the proper implicit uses to be set for that register.

================
Comment at: test/CodeGen/X86/hipe-cc.ll:76
@@ -75,1 +75,3 @@
 
+define cc 11 { i32, i32, i32 } @tailcaller(i32, i32) nounwind {
+  ; CHECK:      movl	$15, %eax
----------------
- Add a comment on what is this function testing.
- Use "opt -instnamer" on this IR to get rid of the [0-9]+ variables.

================
Comment at: test/CodeGen/X86/hipe-cc64.ll:86
@@ -85,1 +85,3 @@
 
+define cc 11 { i64, i64, i64 } @tailcaller(i64, i64) #0 {
+  ; CHECK:      movl	$15, %esi
----------------
Ditto


http://reviews.llvm.org/D21325