[llvm][LV] Drop poison-generating flags from FP instructions

[dcaballe]

Summary

See D111846 for more context. This patch extends Instruction::dropPoisonGeneratingFlags to also
drop poison-generating flags from FP instructions (i.e., nnan and ninf0). Two new tests for nnan and
ninf were pre-committed to LoopVectorize/X86/drop-poison-generating-flags.ll in D112335.

[dcaballe]

• Extended Instruction::dropPoisonGeneratedFlags to support FP
• Updated impacted tests

[spatel]

I didn't go through all of the tests, but the first reduction expansion diff seems unintended to me (see inline comment).
We could enable dropping FMF only with an optional bool parameter? That would allow incremental updates.
Or we handle that as a one-off in the motivating LV caller?

llvm/test/CodeGen/Generic/expand-experimental-reductions.ll

100 ↗

(On Diff #381610)

This isn't the result that we want. This might be due to the vague definitions and interactions of FMF, but I think 'fast' should propagate through this expansion.

For example, if the intrinsic has 'nnan reassoc', then we should be able say that the result is also 'nnan reassoc'. That is, we disallowed NAN inputs and allowed reassociation, so we can assume that a NAN was not accidentally/unknowingly created through this transform where it would not have been in the original code.

The use of dropPoison for integer ops in the ExpandReductions pass is correct though...

[dcaballe]

In D112117#3090902, @spatel wrote:

I didn't go through all of the tests, but the first reduction expansion diff seems unintended to me (see inline comment).
We could enable dropping FMF only with an optional bool parameter? That would allow incremental updates.
Or we handle that as a one-off in the motivating LV caller?

Good catch! I like the optional bool parameter. Honestly, I don't feel very comfortable with dropping all the FMF flags by default since I don't understand in detail all the users of this utility. Let's go with that.

[spatel]

In D112117#3092834, @dcaballe wrote:

In D112117#3090902, @spatel wrote:

I didn't go through all of the tests, but the first reduction expansion diff seems unintended to me (see inline comment).
We could enable dropping FMF only with an optional bool parameter? That would allow incremental updates.
Or we handle that as a one-off in the motivating LV caller?

Good catch! I like the optional bool parameter. Honestly, I don't feel very comfortable with dropping all the FMF flags by default since I don't understand in detail all the users of this utility. Let's go with that.

Sounds good...I was probably too optimistic when I wrote that it would just be minor regressions (perf) if we got this wrong. :)
We need to audit the callers to decide if/when FMF can be propagated.

[dcaballe]

Dropping FMF doesn't seem to be needed after all. See D111846

[reames]

JFYI, I've been taking a look at trying to get FMF flags into the generic dropPoisonGeneratingFlags. So far, the test diff (which looks a lot like this one for the same reasons) has exposed a bunch of cases where the existing code had problems. I'm working through fixing them individually, with the goal of getting a reasonable diff.

The most relevant one which got some discussion on this review is the reduction expansion. That appears to be entirely an artifact of code which a) does nothing without the FMF handling in the generic code, and b) serves no purpose even with it. 0d13f94c and b24db85 simply remove most of it, and fix up some comments.

[dcaballe]

In D112117#3183418, @reames wrote:

JFYI, I've been taking a look at trying to get FMF flags into the generic dropPoisonGeneratingFlags. So far, the test diff (which looks a lot like this one for the same reasons) has exposed a bunch of cases where the existing code had problems. I'm working through fixing them individually, with the goal of getting a reasonable diff.

The most relevant one which got some discussion on this review is the reduction expansion. That appears to be entirely an artifact of code which a) does nothing without the FMF handling in the generic code, and b) serves no purpose even with it. 0d13f94c and b24db85 simply remove most of it, and fix up some comments.

Thanks for the notice, Philip. Based on the feedback that @nlopes provided in D111846, we wouldn't need to drop FMF as part of the vectorization process. That is why I abandoned this review. Maybe you could do something similar to what was suggested above:

We could enable dropping FMF only with an optional bool parameter?

and make sure that the flag is disabled in the vectorizer to preserve what I understand is the expected behavior?

[reames]

The change I'm working on is D115460. To my knowledge, there's no interaction with the vectorizer here, it's simply a code cleanup. e.g. Do the same thing for all types of poison generating flags. We may be too conservative in some places, but consistency has a strong value of it's own. At least per the tests being changed, the vectorizer is not impacted.

[dcaballe]

In D112117#3185648, @reames wrote:

The change I'm working on is D115460. To my knowledge, there's no interaction with the vectorizer here, it's simply a code cleanup. e.g. Do the same thing for all types of poison generating flags. We may be too conservative in some places, but consistency has a strong value of it's own. At least per the tests being changed, the vectorizer is not impacted.

Yes, sorry, I introduced new calls to dropPoisonGeneratingFlags as part of this change that didn't land so the vectorizer shouldn't be impacted by your change. There are some existing calls but they are very specific and shouldn't have a significant impact on FMF.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

 // linearizing the control flow as a result of vectorization.
 static void clearUnsafeFlagsAfterLinearizingCF(Instruction *NewInst) {
   if (isa<OverflowingBinaryOperator>(NewInst)) {
     NewInst->setHasNoUnsignedWrap(false);
     NewInst->setHasNoSignedWrap(false);
   }
   if (isa<PossiblyExactOperator>(NewInst))
     NewInst->setIsExact(false);
+  if (isa<FPMathOperator>(NewInst)) {
+    NewInst->setHasNoNaNs(false);
+    NewInst->setHasNoInfs(false);
+    NewInst->setHasNoSignedZeros(false);
+  }
 }
 
 void InnerLoopVectorizer::scalarizeInstruction(Instruction *Instr,
                                                VPReplicateRecipe *RepRecipe,
                                                const VPIteration &Instance,
                                                bool IfPredicateInstr,
                                                VPTransformState &State) {
   assert(!Instr->getType()->isAggregateType() && "Can't handle vectors");