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

[Oren Ben Simhon via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2810
+  // input arguments.
+  std::stable_sort(ArgLocs.begin(), ArgLocs.end(),
+                   [](const CCValAssign &A, const CCValAssign &B) -> bool {
----------------
rnk wrote:
> The stable_sort is only needed if the CC was vectorcall.
> 
> I think the code would actually be clearer if we used std::merge. After the first pass, ArgLocs should be all the non-HVA argument locations sorted by argument number. The second pass appends additional sorted HVA argument locations. Then you merge those two sorted lists. You'll need a temporary ArgLocs vector to make this work.
> 
> Please factor out this two pass vectorcall code into a template function that operates on a SmallVectorImpl<T>, where T can be InputArg or OutputArg.
I created a template function as you suggested.

The next loop assumes that the locations are sorted. Today Vectorcall CC change the order, tomorrow, some other functionality/CC could change that.
Since we already do the sort, we might as well do it for all CC and avoid future issues.

I agree that merge algorithm could be faster than stable_sort. But the overhead of using it is big.
Not only i will need a temporary ArgLocs, I will need a temporary CCState. IMHO, the current solution is preferred.




================
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}
----------------
rnk wrote:
> oren_ben_simhon wrote:
> > 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)
> > 
> > 
> This was really just a whitebox test to show that LLVM does not crash when you return 5 doubles. We should keep the test if we don't fatal error. Any behavior is fine. For example, it might trigger LLVM's logic to demote return by value to sret.
There is no fatal error. I am fine with leaving the test.


Repository:
  rL LLVM

https://reviews.llvm.org/D27392