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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:308
+static unsigned tryToRepresentAsInstArg(unsigned Opcode, Instruction *I) {
+  if (Opcode != Instruction::PHI &&
+      I->getOpcode() != Instruction::PHI &&
----------------
ABataev wrote:
> dtemirbulatov wrote:
> > ABataev wrote:
> > > Instead, I would check for the supported instructions rather than for the unsupported ones.
> > The other list is a bit long in my implementation, this one looks better.
> I still think it is better to limit this with the list of supported operations rather than the list of the unsupported ones.
ok, I will change that.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:304
+  return (Opcode == Instruction::SRem || Opcode == Instruction::URem ||
+          Opcode == Instruction::SRem || Opcode == Instruction::FRem);
+}
----------------
RKSimon wrote:
> Repeated Instruction::SRem
oh, thanks. I missed that.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1100
+
+  struct PseudoScheduleData : public ScheduleData {
+
----------------
ABataev wrote:
> Very strange that you still need particular scheduling class for the pseudo instructions, I think you can use the original data structure if you correctly implement the pseudo instruction itself. I still don't see all the changes we talked about.
ok, I am implementing now.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1625
+  }
+  llvm_unreachable("unknown binop for default constant value");
+}
----------------
spatel wrote:
> RKSimon wrote:
> > @spatel @dtemirbulatov Can we use getBinOpIdentity yet ?
> Yes - if anyone has suggestions for making that 'AllowRHSConstant' param clearer, let me know. 
> 
> The only problem that I see is that this code is returning +0.0 as the default constant for an fadd (because the caller guarantees 'nsz'?).
> I worked around something like that here:
> rL346143
> 
> I can't tell if SLP would want to do something like that.
ok, yes, looks like it is going to work.


https://reviews.llvm.org/D28907