Page MenuHomePhabricator

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

Authored by dcaballe on Oct 19 2021, 6:53 PM.

Details

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.

Diff Detail

Event Timeline

dcaballe created this revision.Oct 19 2021, 6:53 PM
dcaballe requested review of this revision.Oct 19 2021, 6:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 6:53 PM
dcaballe updated this revision to Diff 381610.Oct 22 2021, 11:26 AM
  • Extended Instruction::dropPoisonGeneratedFlags to support FP
  • Updated impacted tests
dcaballe retitled this revision from [LV] Drop FP flags from instructions that need predication to [llvm][LV] Drop poison-generating flags from FP instructions.Oct 22 2021, 11:26 AM
dcaballe edited the summary of this revision. (Show Details)

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

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...

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.

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 abandoned this revision.Mon, Nov 22, 3:06 AM

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