[PATCH] D61802: [LoopVectorize] Enable float minmax reductions via instruction flag

[Sanjay Patel via Phabricator via llvm-commits]

spatel added reviewers: efriedma, RKSimon.
spatel added a comment.

In D61802#1500473 <https://reviews.llvm.org/D61802#1500473>, @nlw0 wrote:

> I had not run `make check`, sorry I'm still a bit lost. There is a number of broken tests. Is the idea that these might be all be benign changes due to the vectorization not kicking in before, and we might just need to update what the generated code now looks like? And then the test file you already included would now be just another test in such state? How do I update this PR now, just apply the patch over current master and upload a new patch file?


The problem is that the patch does not check that 'I' is an FPMathOperator before asking if 'I->hasNoNaNs()', so we are crashing (asserting) on a bunch of existing tests.

The logic should look like this to work with the current state of FMF:

  diff --git a/llvm/lib/Analysis/IVDescriptors.cpp b/llvm/lib/Analysis/IVDescriptors.cpp
  index 19f1a771b85..b6de4ace07c 100644
  --- a/llvm/lib/Analysis/IVDescriptors.cpp
  +++ b/llvm/lib/Analysis/IVDescriptors.cpp
  @@ -586,8 +586,9 @@ RecurrenceDescriptor::isRecurrenceInstr(Instruction *I, RecurrenceKind Kind,
       LLVM_FALLTHROUGH;
     case Instruction::FCmp:
     case Instruction::ICmp:
  -    if (Kind != RK_IntegerMinMax &&
  -        (!HasFunNoNaNAttr || Kind != RK_FloatMinMax))
  +    bool MightHaveNans = !HasFunNoNaNAttr &&
  +                         I->getOpcode() == Instruction::FCmp && !I->hasNoNaNs();
  +    if (Kind != RK_IntegerMinMax && (Kind != RK_FloatMinMax || MightHaveNans))
         return InstDesc(false, I);
       return isMinMaxSelectCmpPattern(I, Prev);
     }

Unfortunately, this patch is exposing existing problems with our instruction-level fast-math-flags. See:
https://bugs.llvm.org/show_bug.cgi?id=38086
https://bugs.llvm.org/show_bug.cgi?id=39535
https://bugs.llvm.org/show_bug.cgi?id=23116
D51701 <https://reviews.llvm.org/D51701>
https://bugs.llvm.org/show_bug.cgi?id=35538#c2

So the vectorization code already has the potential to miscompile because we can't just check for no-NANs (nnan). We also need to check for no-signed-zeros (nsz). But checking for nsz should happen on the 'select' rather than the 'fcmp'. I think we should fix the fundamental problems in the IR itself instead of trying to hack around them any more.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61802