[PATCH] ARM: Homogeneous aggregates must be allocated to contiguous registers

[James Molloy]

Hi Oliver,

Overall this looks fine to me. I have a bunch of coding style comments, and I think given that you're adding a new argument attribute we need IR-level tests to check that the attribute can be parsed and emitted both as IR and as bitcode.

Cheers,

James

================
Comment at: include/llvm/CodeGen/CallingConvLower.h:340
@@ +339,3 @@
+  /// registers. If this is not possible, return zero. Otherwise, return the first
+  /// registeriof the block that were allocated, marking the entire block as allocated.
+  unsigned AllocateRegBlock(const uint16_t *Regs, unsigned NumRegs, unsigned RegsRequired) {
----------------
Typo: register of

================
Comment at: include/llvm/Target/TargetLowering.h:2223
@@ +2222,3 @@
+  /// must be passed in a block of consecutive registers.
+  virtual bool getFuncArgNeedsRegBlock(Type *Ty, CallingConv::ID CallConv,
+                                       bool isVarArg) const {
----------------
I don't like the name of this function: "functionArgumentNeedsConsecutiveRegisters" might be more explanatory?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7116
@@ +7115,3 @@
+    Type *FinalType = Args[i].Ty;
+    if (Args[i].isByVal) {
+      FinalType = cast<PointerType>(Args[i].Ty)->getElementType();
----------------
Braces not needed here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7119
@@ +7118,3 @@
+    }
+    bool needsRegBlock =
+        getFuncArgNeedsRegBlock(FinalType, CLI.CallConv, CLI.IsVarArg);
----------------
Local variables are UpperCamelCased.

================
Comment at: lib/Target/ARM/ARMCallingConv.h:192
@@ +191,3 @@
+  if (ArgFlags.isInConsecutiveRegsLast()) {
+    assert(PendingHAMembers.size() > 0 && PendingHAMembers.size() <= 4);
+
----------------
Please add a '&& "explanatory text!"' to the assert.

================
Comment at: lib/Target/ARM/ARMCallingConv.h:216
@@ +215,3 @@
+
+    unsigned regResult =
+        State.AllocateRegBlock(RegList, NumRegs, PendingHAMembers.size());
----------------
RegResult

================
Comment at: lib/Target/ARM/ARMCallingConv.h:233
@@ +232,3 @@
+    // Mark all VFP regs as unavailable (AAPCS rule C.2.vfp)
+    for (unsigned regNo = 0; regNo < 16; ++regNo) {
+      State.AllocateReg(SRegList[regNo]);
----------------
Braces not needed

================
Comment at: lib/Target/ARM/ARMCallingConv.h:240
@@ +239,3 @@
+    switch (LocVT.SimpleTy) {
+    case MVT::f32:
+      Size = 4;
----------------
Could you simplify this by using MVT::getSizeInBits()? 

  unsigned Size = LocVT.SimpleTy.getSizeInBits() / 8;
  unsigned Align = LocVT.SimpleTy == MVT::v2f64 ? 8 : Size;

================
Comment at: lib/Target/ARM/ARMCallingConv.h:256
@@ +255,3 @@
+
+    for (SmallVectorImpl<CCValAssign>::iterator It = PendingHAMembers.begin();
+         It != PendingHAMembers.end(); ++It) {
----------------
Should be able to use a range loop here now that we're using C++11.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1244
@@ +1243,3 @@
+      return CallingConv::ARM_APCS;
+    } else if (Subtarget->hasVFP2() && !isVarArg)
+      return CallingConv::ARM_AAPCS_VFP;
----------------
Why are you converting fastcc functions to AAPCS_VFP here? 

================
Comment at: test/CodeGen/ARM/2014-03-04-hfa-in-contiguous-registers.ll:1
@@ +1,2 @@
+; RUN: llc < %s | FileCheck %s
+
----------------
The file name shouldn't contain the date any more, we stopped this a while ago.

http://reviews.llvm.org/D3082