[PATCH] D28907: [SLP] Fix for PR30787: Failure to beneficially vectorize 'copyable' elements in integer binary ops.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Sorry I don't have much to comment yet, but I have some nitpicky comments + (very) small improvement on diff size.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:169
 
-///\returns bool representing if Opcode \p Op can be part
-/// of an alternate sequence which can later be merged as
-/// a ShuffleVector instruction.
-static bool canCombineAsAltInst(unsigned Op) {
-  return Op == Instruction::FAdd || Op == Instruction::FSub ||
-         Op == Instruction::Sub || Op == Instruction::Add;
+/// true if the \p Value is odd, false otherwise.
+static bool isOdd(unsigned Value) {
----------------
No need to document this one. Simple, explanatory name, and static


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:175
+/// Checks if the \p CheckedOpcode is equal \p Opcode or \p AltOpcode.
+static bool sameOpcodeOrAlt(unsigned Opcode, unsigned AltOpcode,
+                            unsigned CheckedOpcode) {
----------------
Nit: I'd rather have `CheckedOpcode` come first. And probably call it "is <something>". Like `isOneOf` or similar (generic name is ok if we only need one function like this). Should be more readable.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:278
+  /// The very first instruction in the list with the main opcode.
+  Value *OpValue = nullptr;
+  /// Some of the instructions in the list have alternate opcodes.
----------------
Put the pointer first, to minimize padding.
Feel free to keep the constructor's parameter list in the current order if it makes more sense, of course.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:345
+         E->getOpcode() == Instruction::ExtractValue);
+  if (E->getOpcode() == Instruction::ExtractElement) {
     ConstantInt *CI = dyn_cast<ConstantInt>(E->getOperand(1));
----------------
All the changes in this function and its callers can be extracted from this diff, AFAICT.


https://reviews.llvm.org/D28907