[PATCH] D122104: [X86][regcall] Support passing / returning structures

[LuoYuanke via Phabricator via cfe-commits]

LuoYuanke added inline comments.


================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:590
+  /// Log 2 of the maximum vector width.
+  unsigned MaxVectorWidth : 4;
+
----------------
I notice some code would indicate it is log 2 size with Log2 suffix in the variable name. Do you think it is more readable to add Log2 suffix?


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5238
+  for (unsigned i = 0; i < IRCallArgs.size(); ++i)
+    LargestVectorWidth = std::max(LargestVectorWidth,
+                                  getMaxVectorWidth(IRCallArgs[i]->getType()));
----------------
Does this also affect other calling convention besides fastcall?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2303
   void classify(QualType T, uint64_t OffsetBase, Class &Lo, Class &Hi,
-                bool isNamedArg) const;
+                bool isNamedArg, bool IsRegCall = false) const;
 
----------------
Update the comments for the new parameter?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:3031
     // than eight eightbytes, ..., it has class MEMORY.
-    if (Size > 512)
+    if (!IsRegCall && Size > 512)
       return;
----------------
Would you add a test for non regcall? Pass 1024 bit vector parameter and check if it is well handled both with regcall and without regcall.
Would you add comments to depict why regcall accept the size which is more than 512?


================
Comment at: clang/test/CodeGen/aarch64-neon-tbl.c:45
 
-// CHECK-LABEL: define{{.*}} <8 x i8> @test_vqtbl2_s8([2 x <16 x i8>] %a.coerce, <8 x i8> noundef %b) #0 {
+// CHECK-LABEL: define{{.*}} <8 x i8> @test_vqtbl2_s8([2 x <16 x i8>] %a.coerce, <8 x i8> noundef %b) #1 {
 // CHECK:   [[__P0_I:%.*]] = alloca %struct.int8x16x2_t, align 16
----------------
I'm curious why aarch64 test cases are affected by the patch.


================
Comment at: clang/test/CodeGen/regcall2.c:2
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -emit-llvm %s -o - -ffreestanding -target-feature +avx512vl -triple=x86_64-pc-win32     | FileCheck %s --check-prefix=Win
+// RUN: %clang_cc1 -emit-llvm %s -o - -ffreestanding -target-feature +avx512vl -triple=x86_64-pc-linux-gnu | FileCheck %s --check-prefix=Lin
----------------
Add test case for target that has no avx512 feature?


================
Comment at: clang/test/CodeGen/regcall2.c:9
+  __m512d r1[4];
+  __m512 r2[4];
+} __sVector;
----------------
May add a test case to show what's the max register we can pass with regcall.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122104