ARM32 Vector lowering - scalarize select

src/IceTargetLowering.h

 //===- subzero/src/IceTargetLowering.h - Lowering interface -----*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 452 matching lines @@
   void _set_dest_redefined() { Context.getLastInserted()->setDestRedefined(); }
 
   bool shouldOptimizeMemIntrins();
 
   void scalarizeArithmetic(InstArithmetic::OpKind K, Variable *Dest,
                            Operand *Src0, Operand *Src1);
 
   /// Generalizes scalarizeArithmetic to support other instruction types.
   ///
   /// MakeInstruction is a function-like object with signature
   /// (Variable *Dest, Variable *Src0, Variable *Src1) -> Instr *.

[John, 2016/02/11 15:02:41]

out of date comment...

that's why I would encourage having an non-dependent type for F. would something
like:

template <typename... Operand>
void scalarizeInstruction(Variable *Dest, std::function<Instr *(Variable *,
Operand *...)> MakeInstruction, Operand *... Src)

work?

-  template <typename F>
-  void scalarizeInstruction(Variable *Dest, Operand *Src0, Operand *Src1,
-                            F &&MakeInstruction) {

[John, 2016/02/11 15:02:41]

I didn't see this before, but I am not a fan of this F &&MakeFunction. Nothing
wrong with lambdas, and unfortunately this is the most efficient way to pass a
lambda, but you are giving up the explicit signature. You are also instantiating
this template with different arguments for each call site. Consider changing
this argument to an std::function and, if this becomes a bottleneck, we can
revisit and change to a dependent type.

+  template <typename F, typename... Operands>
+  void scalarizeInstruction(Variable *Dest, F &&MakeInstruction,

[John, 2016/02/11 15:02:41]

MakeInstruction is a parameter, hence it is named in CamelCaseStyle. I would
like to suggest, though, that we change it to abide by the functionNamingStyle.
Thoughts?

[Eric Holk, 2016/02/11 19:17:23]

That sounds alright to me. I went ahead and made the change.

+                            Operands *... Srcs) {
     const Type DestTy = Dest->getType();
     assert(isVectorType(DestTy));
     const Type DestElementTy = typeElementType(DestTy);
     const SizeT NumElements = typeNumElements(DestTy);
-    const Type Src0ElementTy = typeElementType(Src0->getType());
-    const Type Src1ElementTy = typeElementType(Src1->getType());
-
-    assert(NumElements == typeNumElements(Src0->getType()));
-    assert(NumElements == typeNumElements(Src1->getType()));
 
     Variable *T = Func->makeVariable(DestTy);
     Context.insert<InstFakeDef>(T);
+
     for (SizeT I = 0; I < NumElements; ++I) {
-      Constant *Index = Ctx->getConstantInt32(I);
+      auto *Index = Ctx->getConstantInt32(I);
 
-      // Extract the next two inputs.
-      Variable *Op0 = Func->makeVariable(Src0ElementTy);
-      Context.insert<InstExtractElement>(Op0, Src0, Index);
-      Variable *Op1 = Func->makeVariable(Src1ElementTy);
-      Context.insert<InstExtractElement>(Op1, Src1, Index);
+      auto MakeExtract = [&](Operand *Src) {

[John, 2016/02/11 15:02:41]

please, no default capture -- capture what you need, even if it means capturing
every local plus this.

Also, I know MakeExtract is local, but only capture by reference if you need to
change the local -- otherwise capture by value. Given that everything you need
to capture is a pointer already, capturing by ref is strictly worse.

[Jim Stichnoth, 2016/02/11 15:26:07]

Check whether the experts (hi John!) are OK with this use of default capture.

[Eric Holk, 2016/02/11 19:17:23]

Done.

+        assert(typeNumElements(Src->getType()) == NumElements);
+
+        const auto ElementTy = typeElementType(Src->getType());
+        Variable *Op = Func->makeVariable(ElementTy);

[John, 2016/02/11 15:02:41]

auto here?

[John, 2016/02/11 15:02:41]

why did you use makeVariable instead of makeReg?

[Eric Holk, 2016/02/11 19:17:23]

makeReg is defined in TargetARM32, and this is in the target-independent code.

[Eric Holk, 2016/02/11 19:17:23]

Done.

+        Context.insert<InstExtractElement>(Op, Src, Index);
+        return Op;
+      };
 
       // Perform the operation as a scalar operation.
       Variable *Res = Func->makeVariable(DestElementTy);
-      auto Arith = MakeInstruction(Res, Op0, Op1);
-      // We might have created an operation that needed a helper call.
+      auto *Arith = MakeInstruction(Res, MakeExtract(Srcs)...);

[John, 2016/02/11 15:02:41]

I just noticed that MakeInstruction is misleading. I was reading the code as if
MakeInstruction would only create the instructions -- but it inserts as well.
Consider renaming it to InsertScalarInstruction.

[Eric Holk, 2016/02/11 19:17:23]

Done.

       genTargetHelperCallFor(Arith);

[John, 2016/02/11 15:02:41]

Calling this method here is bad -- really bad.

This method is supposed to be called before lowering starts to pre-generate
calls so that the frame size calculation works.

You probably need to generate the Insert/Op/Extract operations before the
TargetLowering::genTargetHelperCallFors() is invoked.

[Eric Holk, 2016/02/11 19:17:23]

Scalarize is called by genTargetHelperCallsFor, so it's not so bad here. This is
also how it worked back when scalarization only happened for x86.

I added a flag to make sure we only call this during the correct phase.

 
-      // Insert the result into position.
       Variable *DestT = Func->makeVariable(DestTy);
       Context.insert<InstInsertElement>(DestT, T, Res, Index);
       T = DestT;
-    }
-    Context.insert<InstAssign>(Dest, T);
-  }
-
-  template <typename F>
-  void scalarizeUnaryInstruction(Variable *Dest, Operand *Src0,
-                                 F &&MakeInstruction) {
-    const Type DestTy = Dest->getType();
-    assert(isVectorType(DestTy));
-    const Type DestElementTy = typeElementType(DestTy);
-    const SizeT NumElements = typeNumElements(DestTy);
-    const Type Src0ElementTy = typeElementType(Src0->getType());
-
-    assert(NumElements == typeNumElements(Src0->getType()));
-
-    Variable *T = Func->makeVariable(DestTy);
-    Context.insert<InstFakeDef>(T);
-    for (SizeT I = 0; I < NumElements; ++I) {
-      Constant *Index = Ctx->getConstantInt32(I);
-
-      // Extract the next two inputs.
-      Variable *Op0 = Func->makeVariable(Src0ElementTy);
-      Context.insert<InstExtractElement>(Op0, Src0, Index);
-
-      // Perform the operation as a scalar operation.
-      Variable *Res = Func->makeVariable(DestElementTy);
-      auto Arith = MakeInstruction(Res, Op0);
-      // We might have created an operation that needed a helper call.
-      genTargetHelperCallFor(Arith);
-
-      // Insert the result into position.
-      Variable *DestT = Func->makeVariable(DestTy);
-      Context.insert<InstInsertElement>(DestT, T, Res, Index);
-      T = DestT;
     }
     Context.insert<InstAssign>(Dest, T);
   }
 
   /// SandboxType enumerates all possible sandboxing strategies that
   enum SandboxType {
     ST_None,
     ST_NaCl,
     ST_Nonsfi,
   };
@@ skipping 95 matching lines @@
   virtual void lower() {}
 
 protected:
   explicit TargetHeaderLowering(GlobalContext *Ctx) : Ctx(Ctx) {}
   GlobalContext *Ctx;
 };
 
 } // end of namespace Ice
 
 #endif // SUBZERO_SRC_ICETARGETLOWERING_H