[PATCH] D57728: Relax constraints for reduction vectorization

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

Thanks for updating your patch @sanjoy ! LGTM (with a side-note on a follow up patch to take more specific flags in `expandReductions`).
You may want to update the commit message before you commit :)



================
Comment at: lib/CodeGen/ExpandReductions.cpp:124
+                  : getShuffleReduction(Builder, Vec, getOpcode(ID), MRK,
+                                        FastMathFlags::getFast());
     II->replaceAllUsesWith(Rdx);
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57728