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

[Reid Kleckner via Phabricator via llvm-commits]

rnk 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 {
----------------
oren_ben_simhon wrote:
> 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.
> 
> 
I had envisioned that the sort or merge logic would live in the factored out second pass logic that is specific to vectorcall. All conventions other than vectorcall already preserve the invariant that argument locations are ordered by their IR position, which is why I see this sort/merge as being specific to vectorcall. I'd expect readers to be surprised that this sort is necessary, so I'd like to bind it closely with the code that makes the list unsorted.

If you want to document the invariant of the loop below, we can do that with an assertion.

Also, the merge isn't that bad. You should be able to do this:
  unsigned NumFirstPassLocs = ArgLocs.size();
  CCState.AnalyzeFormalArguments(...);
  decltype(ArgLocs) TmpArgLocs;
  std::swap(TmpArgLocs, ArgLocs);
  auto B = TmpArgLocs.begin(), E = TmpArgLocs.end();
  std::merge(B, B + NumFirstPassLocs, B + NumFirstPassLocs, E, ArgLocs.begin());


Repository:
  rL LLVM

https://reviews.llvm.org/D27392