[PATCH] D28566: Disable Callee Saved Registers

[Amjad Aboud via Phabricator via llvm-commits]

aaboud added a comment.

LGTM.
Just few minor comments below.



================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:70
+  /// Contains the updated callee saved register list.
+  /// As opposed to the static list defined in register info,
+  /// all registers that were disabled (in CalleeSaveDisableRegs)
----------------
These comment lines are not aligned to 80 characters.
Please, run clang-format on this?


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:209
 
+  /// Disables the register from the list of CSRs
+  /// I.e. the register will not appear as part of the CSR mask.
----------------
Why did you remove the dot at the end of this line?


================
Comment at: test/CodeGen/X86/DynamicCalleeSavedRegisters.ll:9
+; However, RegCall also says that a register that was used for 
+; passing/retuning argumnets, can be assumed to be used by the callee.
+; In other words, it is no longer a calle saved register.
----------------
"retuning"-> "returning"
"used" -> "modified"


================
Comment at: test/CodeGen/X86/DynamicCalleeSavedRegisters.ll:10
+; passing/retuning argumnets, can be assumed to be used by the callee.
+; In other words, it is no longer a calle saved register.
+; In this case we want to see that %edi and %esi are saved and %esi is assumed
----------------
"calle" -> "callee"


================
Comment at: test/CodeGen/X86/DynamicCalleeSavedRegisters.ll:13
+; to be used by the caller.
+; This is a hipe CC function that doesn't save any register for the caller 
+; of the callee. So we can be sure that there is no other reason to save 
----------------
Can you mentioned how this "hipe CC" function get's its arguments: a0, b0, c0, d0, and e0?


================
Comment at: test/CodeGen/X86/DynamicCalleeSavedRegisters.ll:35
+
+; Make sure that the callee doesn't save paramteres that were passed as arguments.
+define x86_regcallcc {i32, i32, i32} @test_callee(i32 %a0, i32 %b0, i32 %c0, i32 %d0, i32 %e0) nounwind {
----------------
Please explain how the parameters a0, b0, c0, d0, and e0 are expected to be passed to this function, i.e., what register is used for each one?


================
Comment at: test/CodeGen/X86/sse-regcall.ll:40
 ; WIN64-LABEL: testf32_inp
-; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%rsp).*}}  {{#+}} 16-byte Spill
-; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%rsp).*}}  {{#+}} 16-byte Spill
-; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%rsp).*}}  {{#+}} 16-byte Spill
-; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%rsp).*}}  {{#+}} 16-byte Spill
+; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%r(b|s)p).*}}  {{#+}} 16-byte Spill
+; WIN64: movaps {{%xmm(1[2-5])}}, {{.*(%r(b|s)p).*}}  {{#+}} 16-byte Spill
----------------
I believe these tests should be improved, the regular expression is misused in this test.
However, I think it can be done in separate patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D28566