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

[David Majnemer via Phabricator via llvm-commits]

majnemer added inline comments.


================
Comment at: include/llvm/CodeGen/CallingConvLower.h:327
+  /// A shadow allocated register is a register that was allcoated
+  /// but wasn't added to the location list (Locs)
+  /// \returns true if the register was allocated as shadow or false otherwise
----------------
Comments should end with a period.


================
Comment at: include/llvm/CodeGen/CallingConvLower.h:329
+  /// \returns true if the register was allocated as shadow or false otherwise
+  bool IsShadowAllocatedReg(unsigned reg) const;
+
----------------
Variables start with an uppercase letter.


================
Comment at: include/llvm/Target/TargetCallingConv.h:54-62
+    static const uint64_t Hva = 1ULL << 16; ///< HVA field for
+                                            ///< vectorcall
+    static const uint64_t HvaOffs = 16;
+    static const uint64_t HvaStart = 1ULL << 17; ///< HVA structure start
+                                                 ///<  for vectorcall
+    static const uint64_t HvaStartOffs = 17;
+    static const uint64_t SecArgPass = 1ULL << 18; ///< Second argument
----------------
Can we align this code to match its neighbors?


================
Comment at: lib/CodeGen/CallingConvLower.cpp:74
+
+  for (unsigned i = 0, e = Locs.size(); i != e; ++i) {
+    for (MCRegAliasIterator AI(Locs[i].getLocReg(), &TRI, true); AI.isValid();
----------------
Range-based for loop?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7740-7741
+        // passed InReg - is surely an HVA
+        if (CLI.CallConv == CallingConv::X86_VectorCall && 
+          isa<StructType>(FinalType)) {
+          Flags.setHva();
----------------
Formatting looks strange.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:95
+
+  for (auto Reg: RegList) {
+    if (!State.isAllocated(Reg)) {
----------------
Formatting looks strange.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:104
+    }
+    if (Is64bit && (State.IsShadowAllocatedReg(Reg))) {
+      State.addLoc(
----------------
I'd lose these parens.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:132
+  if (!ValVT.isFloatingPoint() &&
+      !(ValVT.isVector() && ValVT.getSizeInBits() >= 128)) {
+
----------------
I don't think these parens are adding anything.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:145-181
+  if (ArgFlags.isHva()) {
+    if (ArgFlags.isHvaStart()) {
+      // Assign shadow GPR register
+      (void)State.AllocateReg(CC_X86_64_VectorCallGetGPRs());
+
+      // Assign shadow XMM register
+      unsigned Reg = State.AllocateReg(CC_X86_VectorCallGetSSEs(ValVT));
----------------
I wonder if this can be refactored to reduce redundancy:
  if (!ArgFlags.isHva() || ArgFlags.isHvaStart()) {
      // Assign shadow GPR register.
      (void)State.AllocateReg(CC_X86_64_VectorCallGetGPRs());

      // Assign XMM register.
      if (unsigned Reg = State.AllocateReg(CC_X86_VectorCallGetSSEs(ValVT))) {
        if (!ArgFlags.isHva())
          State.addLoc(CCValAssign::getReg(ValNo, ValVT, Reg, LocVT, LocInfo));

        // In Vectorcall Calling convention, additional shadow stack can be
        // created on top of the basic 32 bytes of win64.
        // It can happen if the fifth or sixth argument is vector type or HVA.
        // At that case for each argument a shadow stack of 8 bytes is allocated.
        if (Reg == X86::XMM4 || Reg == X86::XMM5)
          State.AllocateStack(8, 8);
      }

      return true;
  }


================
Comment at: lib/Target/X86/X86CallingConv.cpp:162-163
+    // If this is an HVA - Stop the search
+    return true;
+  } else {
+
----------------
Else after return doesn't conform to LLVM style.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:189
+                          ISD::ArgFlagsTy &ArgFlags, CCState &State) {
+  // On the second pass, go through the HVAs only
+  if (ArgFlags.isSecArgPass()) {
----------------
Ditto.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:201
+  if (!ValVT.isFloatingPoint() &&
+      !(ValVT.isVector() && ValVT.getSizeInBits() >= 128)) {
+    return false;
----------------
I don't think these parens add anything.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:206
+  if (ArgFlags.isHva())
+    return true; // If this is an HVA - Stop the search
+
----------------
Comments should end with a period.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:208
+
+  // Assign XMM register
+  if (unsigned Reg = State.AllocateReg(CC_X86_VectorCallGetSSEs(ValVT))) {
----------------
Ditto.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:214
+
+  return false; // No register was assigned - Continue the search
+}
----------------
Ditto.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3276-3277
+  std::stable_sort(ArgLocs.begin(), ArgLocs.end(),
+                   [](const CCValAssign &a, const CCValAssign &b) -> bool {
+                     return a.getValNo() < b.getValNo();
+                   });
----------------
I'd capitalize these variable names.


Repository:
  rL LLVM

https://reviews.llvm.org/D27392