[PATCH] D18492: [X86][SSE] Vectorize a bit (AND/XOR/OR) op if a BUILD_VECTOR has the same op for all their scalar elements.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

Thanks for explaining why this is a good idea. I really don't want us doing more at this layer than is necessary to clean up after other legalization transforms, but your explanation clarifies that this is specifically the intent. Also, the test case updates certainly make it very clear that this situation comes up all the time.

Couple of high level points:

1. Why just AND, OR, and XOR? Maybe also support shifts? (fine to leave a TODO for now)
2. We need to leave documentation of the rationale in comments as well. I marked where it seems to fit below.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6628-6630
@@ -6627,1 +6627,5 @@
 
+/// If a BUILD_VECTOR's source elements all apply the same bit operation and
+/// one of their operands is constant, lower to a pair of BUILD_VECTOR and
+/// just apply the bit operation to the vectors.
+static SDValue lowerBuildVectorToBitOp(SDValue Op, SelectionDAG &DAG) {
----------------
This seems like a good place to add comments documenting why its important to do this at this level.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6660-6665
@@ +6659,8 @@
+
+  if (SDValue V = LowerAsBitOp(ISD::AND))
+    return V;
+  if (SDValue V = LowerAsBitOp(ISD::XOR))
+    return V;
+  if (SDValue V = LowerAsBitOp(ISD::OR))
+    return V;
+  return SDValue();
----------------
I think using the lambda helper here is a bit overkill. It forces us to do a lot of work that we don't really need to do IMO.

I would get the first element opcode, check that it is one of AND, OR, or XOR, check that it is legal on the target.

Then do the lowering with that opcode.

Shouldn't need a lambda at all, and can use either a switch or 3 tests against ==. I actually think even the switch is likely overkill for just 3 opcodes.


Repository:
  rL LLVM

http://reviews.llvm.org/D18492