Subzero: ARM32: lowering of vector insert and extract.

src/IceInstARM32.cpp

Index: src/IceInstARM32.cpp
diff --git a/src/IceInstARM32.cpp b/src/IceInstARM32.cpp
index 93bbb2769b2db2e1e7027c614765415e557e2856..1b8b2c74974d88049ca4019a842d0776473ebcb4 100644
--- a/src/IceInstARM32.cpp
+++ b/src/IceInstARM32.cpp
@@ -20,7 +20,6 @@
 #include "IceCfgNode.h"
 #include "IceInst.h"
 #include "IceOperand.h"
-#include "IceRegistersARM32.h"
 #include "IceTargetLoweringARM32.h"
 
 namespace Ice {
@@ -981,6 +980,140 @@ InstARM32Mov::InstARM32Mov(Cfg *Func, Variable *Dest, Operand *Src,
   }
 }
 
+RegARM32::AllRegisters
+InstARM32ExtractInsert::getDRegister(Variable *Src, uint32_t VectorSize) const {
+  assert(Src->hasReg());
+  auto SrcReg = Src->getRegNum();

[Jim Stichnoth, 2016/02/03 15:28:37]

You will get some disagreement within the team, but I would like you to back off
on use of auto.

My approximate rules for when to use auto:

1. Something like a cast operation (static_cast, llvm::dyn_cast) where the
explicit type is completely redundant.

  const auto *Var = llvm::dyn_cast<Variable>(Opnd);

2. Other operations where the the type is pretty clear and practically redundant
from context.

  auto *Instr = InstFoo::create();

3. Situations where the actual type is obnoxiously verbose for no good reason.

  auto *Iter = MyContainer.begin();
  auto Func = /* some lambda expression */;

Otherwise, code starts to become write-only and it gets really hard to make
sense of the actual types without continual scrutinizing of header files.

[John, 2016/02/03 16:06:51]

const auto

[Eric Holk, 2016/02/03 21:02:21]

I meant to remove most of my uses of auto before submitting this CL. I'm
generally a fan of auto since I've also spent a lot of time writing in
type-inferred languages, but the local style here is definitely to avoid using
it.

[Eric Holk, 2016/02/03 21:02:21]

Done.

+
+  const auto &RegEntry = RegARM32::RegTable[SrcReg];
+  assert(RegEntry.IsVec128);
+
+  // This code assumes the Aliases list goes Q_n, S_2n, S_2n+1. The asserts in

[Jim Stichnoth, 2016/02/03 15:28:37]

I would really like to stay away from any assumptions on ordering of the
register enum values or the Aliases list.

IIUC, the purpose of mapping from Q register to D registers is for syntactic
correctness of the emit() output, but ultimately in the emitIAS() output you
will just take the encoded register value of the Q register and apply a simple
transformation that has nothing to do with our various orderings.

So I'm thinking maybe this method should return the encoded D register value
instead of the enum value, and the emit() code could then call another function
that returns a register name given the encoded value and register class/type
(presumably f64 in this case because we know that's how to get at D registers).

[John, 2016/02/03 16:06:51]

he will actually need to provide a D register to the underlying assembler. I am
concerned about this as well, so I asked him to leave a TODO behind (see below)
until we come up with something better that does not require assumptions or
clever tricks.

[Eric Holk, 2016/02/03 21:02:21]

I've added the TODO.

+  // the next two branches help to check that this is still true.
+  auto HalfWidth = VectorSize / 2;

[John, 2016/02/03 16:06:51]

optional: I am very auto-lenient, but this is probably too much

[Eric Holk, 2016/02/03 21:02:21]

I removed it. HalfWidth was only used in one place, so I just replaced it with
VectorSize / 2.

+  if (Index < HalfWidth) {
+    SrcReg = RegEntry.Aliases[1];
+    // We have a Q register that's made up of two D registers. This assert is
+    // to help ensure that we picked the right D register.
+    assert(RegARM32::RegTable[RegEntry.Aliases[1]].Encoding + 1 ==
+           RegARM32::RegTable[RegEntry.Aliases[2]].Encoding);
+  } else {
+    SrcReg = RegEntry.Aliases[2];
+    // We have a Q register that's made up of two D registers. This assert is
+    // to help ensure that we picked the right D register.
+    assert(RegARM32::RegTable[RegEntry.Aliases[2]].Encoding - 1 ==
+           RegARM32::RegTable[RegEntry.Aliases[1]].Encoding);
+  }
+  return (RegARM32::AllRegisters)SrcReg;
+}
+
+uint32_t InstARM32ExtractInsert::getDIndex(uint32_t VectorSize) const {
+  if (Index < VectorSize / 2) {
+    return Index;
+  } else {
+    return Index - (VectorSize / 2);
+  }
+}
+
+RegARM32::AllRegisters
+InstARM32ExtractInsert::getSRegister(Variable *Src) const {

[Jim Stichnoth, 2016/02/03 15:28:37]

Same comment as above.

[Eric Holk, 2016/02/03 21:02:21]

Acknowledged.

+  assert(Src->hasReg());
+  auto SrcReg = Src->getRegNum();
+
+  // For floating point values, we hope we got allocated to Q0 - Q7, so we can

[John, 2016/02/03 16:06:51]

I would rephrase this. "hoping" conveys the idea that we cross our fingers that
Src was allocated the right register.

also, won't this assertion fail for non-fp types if, e.g., q15 is allocated? (I
had to search for the uses of getSRegisters() below to figure out that this will
not happen... maybe assert(Src->getType() == v4f32)?)

[Eric Holk, 2016/02/03 21:02:21]

I fixed the comment. I wrote that before the higher up code was actually
guaranteeing that invariant.

I strengthened the assert too. This function should only be called on the v4f32
version anyway.

+  // directly access the value we want as one of the S registers.
+  assert(SrcReg < RegARM32::Reg_q8);
+
+  // This part assumes the register alias list is goes q0, d0, d1, s0, s1, s2,
+  // s3.
+  assert(Index < 4);
+
+  return (RegARM32::AllRegisters)RegARM32::RegTable[SrcReg].Aliases[Index + 3];

[John, 2016/02/03 16:06:51]

I fear what will happen if the alias declaration ever changes. Please leave a
TODO(jpp), and describe that we need to make this more resilient to changes in
the alias declaration.

[Eric Holk, 2016/02/03 21:02:21]

Done.

+}
+
+InstARM32Extract::InstARM32Extract(Cfg *Func, Variable *Dest, Variable *Src,
+                                   uint32_t Index, CondARM32::Cond Predicate)
+    : InstARM32ExtractInsert(Func, Dest, Index, Predicate) {
+  addSource(Src);
+}
+
+void InstARM32Extract::emit(const Cfg *Func) const {
+  auto &Str = Func->getContext()->getStrEmit();
+  auto DestTy = getDest()->getType();
+
+  assert(llvm::isa<Variable>(getSrc(0)));
+  auto Src = llvm::dyn_cast<Variable>(getSrc(0));

[Jim Stichnoth, 2016/02/03 15:28:37]

You can probably just use llvm::cast<> here and remove the assert.

[Eric Holk, 2016/02/03 21:02:21]

Done.

+
+  if (isIntegerType(DestTy)) {
+    Str << "\t"
+        << "vmov" << getPredicate();
+    auto BitSize = typeWidthInBits(DestTy);

[John, 2016/02/03 16:06:51]

instead of adding this new method, why don't you just do

auto BitSize = typeWidthInBytes() * CHAR_BIT

?

[Eric Holk, 2016/02/03 21:02:21]

Done.

+    if (BitSize < 32) {
+      Str << ".s" << BitSize;
+    } else {
+      Str << "." << BitSize;
+    }
+    Str << "\t";
+    getDest()->emit(Func);
+    Str << ", ";
+
+    auto VectorSize = typeNumElements(Src->getType());
+
+    auto SrcReg = getDRegister(Src, VectorSize);
+
+    Str << RegARM32::RegTable[SrcReg].Name;
+    Str << "[" << getDIndex(VectorSize) << "]";
+  } else if (isFloatingType(DestTy)) {
+    auto SrcReg = getSRegister(Src);
+
+    Str << "\t"
+        << "vmov" << getPredicate() << ".f32"
+        << "\t";
+    getDest()->emit(Func);
+    Str << ", " << RegARM32::RegTable[SrcReg].Name;
+  } else {
+    assert(false && "Invalid extract type");
+  }
+}
+
+InstARM32Insert::InstARM32Insert(Cfg *Func, Variable *Dest, Variable *Src,
+                                 uint32_t Index, CondARM32::Cond Predicate)
+    : InstARM32ExtractInsert(Func, Dest, Index, Predicate) {
+  addSource(Src);
+}
+
+void InstARM32Insert::emit(const Cfg *Func) const {
+  auto &Str = Func->getContext()->getStrEmit();
+  auto Dest = getDest();
+  auto DestTy = getDest()->getType();
+
+  assert(llvm::isa<Variable>(getSrc(0)));
+  auto Src = llvm::dyn_cast<Variable>(getSrc(0));
+
+  if (isIntegerType(DestTy)) {
+    Str << "\t"
+        << "vmov" << getPredicate();
+    auto BitSize = typeWidthInBits(typeElementType(DestTy));
+    Str << "." << BitSize << "\t";
+
+    auto VectorSize = typeNumElements(DestTy);
+    auto DestReg = getDRegister(Dest, VectorSize);
+    auto Index = getDIndex(VectorSize);
+    Str << RegARM32::RegTable[DestReg].Name;
+    Str << "[" << Index << "], ";
+    Src->emit(Func);
+  } else if (isFloatingType(DestTy)) {
+    Str << "\t"
+        << "vmov" << getPredicate() << ".f32"
+        << "\t";
+    auto DestReg = getSRegister(Dest);
+    Str << RegARM32::RegTable[DestReg].Name << ", ";
+    Src->emit(Func);
+  } else {
+    assert(false && "Invalid insert type");
+  }
+}
+
 template <InstARM32::InstKindARM32 K>
 void InstARM32CmpLike<K>::emitIAS(const Cfg *Func) const {
   emitUsingTextFixup(Func);