[PATCH] D17427: [X86ISelLowering] Fix TLSADDR lowering when shrink-wrapping is enabled

[Quentin Colombet via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Hi Davide,

LGTM with a few nitpicks on the test case.

Thanks for fixing it!
Cheers,
-Quentin


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:22680
@@ +22679,3 @@
+
+  // Emit CALLSEQ_END  right after the instruction.
+  // We don't call erase from parent because we want to keep the
----------------
two spaces after CALLSEQ_END.

================
Comment at: test/CodeGen/X86/tls-shrink-wrapping.ll:9
@@ +8,3 @@
+; Function Attrs: nounwind uwtable
+define i32 @g() #0 {
+entry:
----------------
Add a comment on what this is testing.
In case the test breaks in the future, eg because of code gen changes, we want to know what was the intended behavior.

================
Comment at: test/CodeGen/X86/tls-shrink-wrapping.ll:11
@@ +10,3 @@
+entry:
+  %0 = load i32, i32* @i, align 4, !tbaa !2
+  %tobool = icmp eq i32 %0, 0
----------------
Please use opt -instnamer on the test case to have temporary variable names (easier to update than %[0-9]+).

================
Comment at: test/CodeGen/X86/tls-shrink-wrapping.ll:11
@@ +10,3 @@
+entry:
+  %0 = load i32, i32* @i, align 4, !tbaa !2
+  %tobool = icmp eq i32 %0, 0
----------------
qcolombet wrote:
> Please use opt -instnamer on the test case to have temporary variable names (easier to update than %[0-9]+).
Please get rid of the metadata (!…) unless they are required to reproduce the problem.

================
Comment at: test/CodeGen/X86/tls-shrink-wrapping.ll:55
@@ +54,3 @@
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.9.0 (trunk 261236)"}
----------------
You should be able to remove all the !… stuff after removing the !… from the function.


http://reviews.llvm.org/D17427