[PATCH] D27392: Vectorcall Calling Convention - Adding CodeGen Complete Support
David Majnemer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 8 09:10:03 PST 2016
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
More information about the llvm-commits
mailing list