[PATCH] D30086: Add generic IR vector reductions

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Right, this is looking much better. Now, what about tests?

We'd probably need a bunch of tests to make sure that the intrinsics are accepted in the syntax that they're documented and rejected otherwise.

I'm not sure how's the best way forward, but probably just having IR in the right/wrong format and passing `-validate` or something expecting it to pass/fail would be a start.

cheers,
--renato



================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1273
+    return getSimpleRdx(Instruction::Xor);
+  case RecurrenceDescriptor::RK_IntegerMinMax: {
+    switch (Desc.getMinMaxRecurrenceKind()) {
----------------
aemerson wrote:
> rengolin wrote:
> > I may be wrong, but I think the LLVM style could be to put this curly brackets on a new line.
> > 
> > clang-format would know better. :)
> This is the LLVM style according to clang-format.
ok


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1293
+  case RecurrenceDescriptor::RK_FloatMinMax: {
+    Flags.IsMaxOp =
+        Desc.getMinMaxRecurrenceKind() == RecurrenceDescriptor::MRK_FloatMax;
----------------
aemerson wrote:
> rengolin wrote:
> > Isn't this `Flags` a local variable? Where is this being used after the call to `createSimpleTargetReduction`?
> It's captured by reference by the `getSimpleRdx` lambda.
ah, yes.


Repository:
  rL LLVM

https://reviews.llvm.org/D30086