Subzero: ARM32: lowering of vector insert and extract.

src/IceInstARM32.h

Index: src/IceInstARM32.h
diff --git a/src/IceInstARM32.h b/src/IceInstARM32.h
index 21505a185045fd2267d2d80ddf293bb971cbf65a..b81c02337fd513c6586baa849b0e826d46efdbd7 100644
--- a/src/IceInstARM32.h
+++ b/src/IceInstARM32.h
@@ -23,6 +23,7 @@
 #include "IceInst.h"
 #include "IceInstARM32.def"
 #include "IceOperand.h"
+#include "IceRegistersARM32.h"
 
 namespace Ice {
 namespace ARM32 {
@@ -389,6 +390,8 @@ public:
     Cmp,
     Dmb,
     Eor,
+    Extract,
+    Insert,
     Label,
     Ldr,
     Ldrex,
@@ -1353,6 +1356,72 @@ private:
   Variable *DestHi = nullptr;
 };
 
+/// Common methods between InstARM32Extract and InstARM32Insert.
+class InstARM32ExtractInsert : public InstARM32Pred {
+  InstARM32ExtractInsert() = delete;
+  InstARM32ExtractInsert(const InstARM32ExtractInsert &) = delete;
+  InstARM32ExtractInsert &operator=(const InstARM32ExtractInsert &) = delete;
+
+protected:
+  InstARM32ExtractInsert(Cfg *Func, Variable *Dest, uint32_t Index,
+                         CondARM32::Cond Predicate)
+      : InstARM32Pred(Func, InstARM32::Extract, 2, Dest, Predicate),

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

Since both Insert and Extract have a single source Operand (i.e., a single call
to addSource()), I would pass it in to this base class ctor and do the
addSource() here.

Also, "2" is the max number of addSource() calls, so make it "1" instead.

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

Done.

Originally I thought there was going to be more divergence between the two
subclasses, but I guess now.

+        Index(Index) {}
+
+  uint32_t Index;

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

const uint32_t Index

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

const

also, why don't you add a

const uint32_t NumElements

?

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

It wasn't immediately clear what the right NumElements was at this point,
although the higher level constructor knows, so that's not a big problem.

The floating point version of this instruction doesn't need NumElements, because
it's always 4.

It doesn't matter much to me, but it's not hard to compute NumElements and it
isn't always needed, so it seems better not to bother storing it here.

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

Done.

+
+  // These next two functions find the D register that maps to the half of the Q
+  // register that this instruction is accessing.
+  RegARM32::AllRegisters getDRegister(Variable *Src, uint32_t VectorSize) const;

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

This isn't absolutely clear-cut, but I think it would be a bit better if these
three functions can figure out on their own what Src and VectorSize should be,
and not have to pass them as arguments.

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

I wasn't totally thrilled with the design I used here.

The challenge is that the source and vector size are determined by the
destination for insertelement, but by the source for extractelement. The common
code doesn't know which version it is, so you'd basically have to duplicate this
code in each of the subclasses.

Perhaps the better design is to make a function that's not part of the class
that takes a register, numelements and an index. As it is right now, it's
strange that the register and numelements are given explicitly but the index is
implicit.

It would actually be pretty easy to compute NumElements from the Src though,
which would at least make it a little cleaner. I went ahead and made this
change, but I'm open to making more changes.

[Jim Stichnoth, 2016/02/03 22:32:21]

I was thinking that the common code could determine whether to use Dest or Src
based on whether the instruction kind is Insert or Extract.

[John, 2016/02/04 15:23:57]

what about defining a virtual method in this class:

Variable *getVectorOperand() const;

and implement getDRegister in terms of that? I believe that's all you'd need.

Alternatively, you could have a

const Variable *VectorOperand;

and let the base classes set it to the right operand. (I'd prefer this approach
to avoid the virtual call.)

[John, 2016/02/04 15:23:57]

Another issue I'd like to point out here. if this method takes no parameter,
then I'd say getDRegister does not convey the idea of what this does. I would
provably implement this helper (and all the other helpers in this base class) as
global functions in anonymous namespaces in the .cpp file.

[Jim Stichnoth, 2016/02/04 16:44:12]

I recalled that in general, we had tried to avoid virtual methods in Inst and
Operand (particularly Variable), to keep object sizes smaller by avoiding
vtables.

Unfortunately I see that Inst has a bunch of integral virtual methods, so oh
well.  Adding a virtual method is probably a fine approach.  Also, this would
not be a virtual method in the base class, so even if we were generally free and
clear of virtual methods, the impact here would be small.
I don't think precisely this is a good idea.  There is essentially an assumption
that all Variable* are captured by FOREACH_VAR_IN_INST().  Probably nothing
would break today by adding a "hidden" Variable*, but...  Maybe better would be
Variable **VectorOperand, with appropriate const's thrown in?

[John, 2016/02/04 16:52:24]

I am **not** suggesting this member in lieu of the canonical Srcs array; See,
for example, X86OperandMem: those classes (X8632 and X8664) will cache Base, and
Index, but they will **also** add them to Srcs.

You don't need a double pointer; a single pointer:

Variable *const Operand;

+  uint32_t getDIndex(uint32_t VectorSize) const;

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

VectorSize to me is always 128-bit. Maybe rename these to NumElements?

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

Done.

+
+  // For floating point values, we can read directly from an S register. This
+  // function finds the right one.
+  RegARM32::AllRegisters getSRegister(Variable *Src) const;
+};

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

Implement classof() for completeness.

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

Does it make sense to have classof here, given that this class should never be
instantiated except as a base of InstARM32Extract and InstARM32Insert (which
both implement classof)? For example, InstArm32Pred does not include classof,
that I can see.

[Jim Stichnoth, 2016/02/03 22:32:21]

If you aren't already aware, have a look at
http://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html .

In this case, the implementation should look something like:

static bool classof(const Inst *Inst) {
  return isClassof(Inst, Insert) || isClassof(Inst, Extract);
}

I didn't know InstArm32Pred was incomplete, maybe it should be fixed? :)

For example, see InstTarget, Constant, Variable.

[Eric Holk, 2016/02/04 20:57:25]

Ah, that makes sense. Thanks.

+
+/// Generates vmov Rd, Dn[x] instructions, and their related floating point
+/// versions.
+class InstARM32Extract final : public InstARM32ExtractInsert {
+  InstARM32Extract() = delete;
+  InstARM32Extract(const InstARM32Extract &) = delete;
+  InstARM32Extract &operator=(const InstARM32Extract &) = delete;
+
+public:
+  static InstARM32Extract *create(Cfg *Func, Variable *Dest, Variable *Src,
+                                  uint32_t Index, CondARM32::Cond Predicate) {
+    return new (Func->allocate<InstARM32Extract>())
+        InstARM32Extract(Func, Dest, Src, Index, Predicate);
+  }
+  void emit(const Cfg *Func) const override;
+  static bool classof(const Inst *Inst) { return isClassof(Inst, Extract); }
+
+private:
+  InstARM32Extract(Cfg *Func, Variable *Dest, Variable *Src, uint32_t Index,
+                   CondARM32::Cond Predicate);
+};
+
+/// Generates vmov Dn[x], Rd instructions, and their related floating point
+/// versions.
+class InstARM32Insert final : public InstARM32ExtractInsert {
+  InstARM32Insert() = delete;
+  InstARM32Insert(const InstARM32Insert &) = delete;
+  InstARM32Insert &operator=(const InstARM32Insert &) = delete;
+
+public:
+  static InstARM32Insert *create(Cfg *Func, Variable *Dest, Variable *Src0,
+                                 uint32_t Index, CondARM32::Cond Predicate) {
+    return new (Func->allocate<InstARM32Insert>())
+        InstARM32Insert(Func, Dest, Src0, Index, Predicate);
+  }
+  void emit(const Cfg *Func) const override;
+  static bool classof(const Inst *Inst) { return isClassof(Inst, Insert); }
+
+private:
+  InstARM32Insert(Cfg *Func, Variable *Dest, Variable *Src0, uint32_t Index,

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

This parameter is named Src0, the one for Extract is name Src; make consistent.

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

Done.

+                  CondARM32::Cond Predicate);
+};
+
 class InstARM32Vcmp final : public InstARM32Pred {
   InstARM32Vcmp() = delete;
   InstARM32Vcmp(const InstARM32Vcmp &) = delete;