[PATCH] D57728: Relax constraints for reduction vectorization

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy marked 2 inline comments as done.
sanjoy added inline comments.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:124
+                  : getShuffleReduction(Builder, Vec, getOpcode(ID), MRK,
+                                        FastMathFlags::getFast());
     II->replaceAllUsesWith(Rdx);
----------------
sdesmalen wrote:
> sanjoy wrote:
> > sdesmalen wrote:
> > > sdesmalen wrote:
> > > > Rather than choosing 'getFast() and going through 'IsOrdered' (which is set when the full 'fast' property is set), do we want to take the FastMathFlags from the IntrinsicInst directly?
> > > Note that there is a test missing for this change.
> > > Rather than choosing 'getFast() and going through 'IsOrdered' (which is set when the full 'fast' property is set), do we want to take the FastMathFlags from the IntrinsicInst directly?
> > 
> > My rationale was that I wanted to keep this as obviously NFC as possible -- previously we would implicitly tag the instructions as `fast`, and this change just makes it explicit.
> > 
> > > Note that there is a test missing for this change.
> > 
> > This change is not supposed to change any behavior (assuming no bugs :) ).  Do you want me to add a test case to check existing behavior?  `CodeGen/Generic/expand-experimental-reductions.ll` already tests for `fast` flags in the expansion.
> > My rationale was that I wanted to keep this as obviously NFC as possible -- previously we would implicitly tag the instructions as fast, and this change just makes it explicit.
> In a way that feels a bit artificial since the patch is already not NFC and the purpose of this patch is to be more specific about passing the exact flags. However, if you think it makes more sense to do this change in a separate patch, that's fine with me.
As I said above, we already have tests for this behavior (that we propagate fast-math flags into `getShuffleReduction`.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57728/new/

https://reviews.llvm.org/D57728