[PATCH] D44374: [GlobalISel] Add support for lowering vector operations to scalar.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerHelper.h:91
+  /// as scalar operations.
+  LegalizeResult scalarElementsVector(MachineInstr &MI, unsigned TypeIdx);
+
----------------
Just a nit pick: I think "scalarizeVector" sounds more natural and in a similar style as the other methods here than "scalarElementsVector".
What do you think?


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:75
+  /// 8 separate s64 adds.
+  LowerScalar,
+
----------------
I think that similarly, "Scalarize" here might be less confusing that "LowerScalar".
"WidenScalar" and "NarrowScalar" already exist and do something on Scalar data types - for "LowerScalar" one could then also guess it does something on scalar data types which it doesn't. I think "Scalarize" is less ambiguous.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371
+  /// Lower the element to scalar operations of the same element type
+  LegalizeRuleSet &lowerScalarFor(std::initializer_list<LLT> Types) {
+    return actionFor(LegalizeAction::LowerScalar, Types);
----------------
To be consistent with earlier comments: maybe best to change the method name to "scalarizeFor"?


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1075
+    int NumElements = Size / ElementSize;
+    assert(Size % ElementSize == 0 && "Don't know how to handle situation where vector isn't the same size");
+
----------------
This is over 80 characters long. Please run clang-format on the whole patch.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1081
+    extractParts(MI.getOperand(1).getReg(), ElementTy, NumElements, Src1Regs);
+    extractParts(MI.getOperand(2).getReg(), ElementTy, NumElements, Src2Regs);
+
----------------
I would have assumed that G_FMA has Operands 0, 1, 2, and 3; but I see only Operands 0, 1 and 2 being handled here.
I guess either G_FMA must be removed as a case from this switch statement and an assert added here that the number of operands is exactly 3; or the code should be made more generic to be able to handle generic opcodes with more input operands?


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerInfoTest.cpp:206-210
+  for (unsigned Elements = 2; Elements < 16; ++Elements) {
+    for (unsigned Size : {8, 16, 32, 64}) {
+      L.setAction({G_FADD, 0, LLT::vector(Elements, Size)}, LowerScalar);
+    }
+  }
----------------
This suggests that you need to call the setAction API for every combination of (number of elements, element size) vector.
That doesn't scale.
Can you rewrite this in a simple way to represent that all vectors - irrespective of number of elements or element size - need to be scalarized for a given opcode?
e.g. something similar to the setLegalizeVectorElementToDifferentSizeStrategy API in tests higher up in this function.
Maybe a new function call in the API at that level needs to be introduced to be able to represent this?


Repository:
  rL LLVM

https://reviews.llvm.org/D44374