[PATCH] D27392: Vectorcall Calling Convention - Adding CodeGen Complete Support

[Oren Ben Simhon via Phabricator via llvm-commits]

oren_ben_simhon marked 21 inline comments as done.
oren_ben_simhon added inline comments.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:65
+                                           X86::ZMM3, X86::ZMM4, X86::ZMM5};
+    return makeArrayRef(std::begin(RegListZMM), std::end(RegListZMM));
+  }
----------------
rnk wrote:
> ArrayRefs are implicitly constructable from C arrays. You should be able to just return RegListZMM here and throughout this file. If not, makeArrayRef takes C arrays and will do the right thing.
You are right. Thank you.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:95
+
+  for (auto Reg: RegList) {
+    if (!State.isAllocated(Reg)) {
----------------
majnemer wrote:
> Formatting looks strange.
I ran clang format before uploading the file. I added comments, hopefully it makes the formatting look better.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:132
+  if (!ValVT.isFloatingPoint() &&
+      !(ValVT.isVector() && ValVT.getSizeInBits() >= 128)) {
+
----------------
majnemer wrote:
> I don't think these parens are adding anything.
Consider a case in which ValVT is a vector type.
If i will remove these parens, i will enter the if statement block.
However i do not want to enter this statement in the case of vectors type .
That is why this parens are not redundant.

I will change a bit the condition look to make it clearer.


================
Comment at: lib/Target/X86/X86CallingConv.td:631
-
-  // 256-bit vectors use YMM registers.
-  CCIfType<[v32i8, v16i16, v8i32, v4i64, v8f32, v4f64],
----------------
rnk wrote:
> Ouch. I locally confirmed this is correct, but why design a new calling convention that doesn't handle the latest vector types... =P
If you refer to 512 bit vector types, Microsoft probably forgot to mention that it should be assigned to ZMM [[ https://msdn.microsoft.com/en-us/library/dn375768.aspx | here ]].


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2807
+
+  // Sort the the locations of the arguments according to their original
+  // position
----------------
aaboud wrote:
> You have a typo: "the the".
> Also, can you explain why you are sorting the ArgLoc even for non-VectorCall calling convention?
> Is that needed?
The next loop assumes that the locations are in the same order of the input arguments.
So the order should be kept for all calling conventions.

Currently AFAIK, vectorcall is the only one that changes the arguments order, still additional calling conventions might do the same.

Anyway, I will add a comment to clarify this.


================
Comment at: test/CodeGen/X86/vectorcall.ll:71
-; tablegen any other way.
-define x86_vectorcallcc {double, double, double, double, double} @test_fp_4() {
-  ret {double, double, double, double, double}
----------------
aaboud wrote:
> Do you really want to remove this test? Cannot you just fix it?
AFAIK, This test checks something that cannot happen.
There is no case in which a function will return {double, double, double, double, double}.
If it wanted to return such a structure it will be returned by pointer as the first argument to the function:

> define x86_vectorcallcc void @test_fp_4(%struct.five_doubles* sret %a)




Repository:
  rL LLVM

https://reviews.llvm.org/D27392