[PATCH] D96415: [PowerPC][AIX] Support passing vector arguments on the stack.

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6402-6403
     else {
-      report_fatal_error(
-          "passing vector parameters to the stack is unimplemented for AIX");
+      const unsigned VecSize = 16;
+      const unsigned Offset = State.AllocateStack(VecSize, Align(VecSize));
+      State.addLoc(CCValAssign::getMem(ValNo, ValVT, Offset, LocVT, LocInfo));
----------------
Do we need the `VecSize` variable here instead of just using `16`?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vec-arg-spills-mir.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mattr=+altivec -vec-extabi \
----------------
Does the script work with AIX now? It would make things a lot easier. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vec-arg-spills-mir.ll:15
+define double @caller() {
+  ; MIR32-LABEL: name: caller
+  ; MIR32: bb.0.entry:
----------------
Format to the beginning of the line. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vec-arg-spills-mir.ll:78
+  ; MIR32:   BLR implicit $lr, implicit $rm, implicit $f1
+  ; MIR64-LABEL: name: caller
+  ; MIR64: bb.0.entry:
----------------
Maybe an empty line separating the 32bit and 64bit would be better. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vec-arg-spills.ll:82
+; 32BIT-NEXT:    blr
+;
+; 64BIT-LABEL: caller:
----------------
`;` can be removed, I think.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vector-stack.ll:1
-; RUN: not --crash llc < %s -verify-machineinstrs -mcpu=pwr7 -mattr=+altivec \
-; RUN:   -vec-extabi -mtriple powerpc-ibm-aix-xcoff 2>&1 | \
-; RUN: FileCheck %s --check-prefix=AIX-ERROR
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mattr=+altivec \
+; RUN:     -vec-extabi -mtriple powerpc-ibm-aix-xcoff < %s | \
----------------
I think we can remove this test? The ones you added do a much better job of showing what the ABI does. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96415/new/

https://reviews.llvm.org/D96415