This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorizer] Require no-signed-zeros-fp-math=true for fmin/fmax
ClosedPublic

Authored by kmclaughlin on Feb 12 2021, 6:52 AM.

Details

Summary

Currently, setting the no-nans-fp-math attribute to true will allow
loops with fmin/fmax to vectorize, though we should be requiring that
no-signed-zeros-fp-math is also set.

This patch adds the check for no-signed-zeros at the function level and includes
tests to make sure we don't vectorize functions with only one of the attributes
associated.

Diff Detail

Event Timeline

kmclaughlin created this revision.Feb 12 2021, 6:52 AM
kmclaughlin requested review of this revision.Feb 12 2021, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 6:52 AM
spatel added inline comments.Feb 12 2021, 7:20 AM
llvm/lib/Analysis/IVDescriptors.cpp
597–599

That logic is hard to follow. How about inverting the condition, so we are positively identifying a min/max candidate?

I'm thinking:

if (isIntMinMaxRecurrenceKind(Kind) ||
    (FMF.noNans() && FMF.noSignedZeros() && 
     isFPMinMaxRecurrenceKind(Kind)))
  return isMinMaxSelectCmpPattern(I, Prev);
llvm/test/Transforms/LoopVectorize/float-minmax-instruction-flag.ll
151

I know we have an FMF mess currently, but can you add a test comment here to explain that this should not vectorize even after we switch to IR-level FMF? Or if there are other tests already checking for that scenario, point to those.

kmclaughlin marked 2 inline comments as done.
  • Reordered the conditions in isRecurrenceInstr
  • Added a comment to explain the @minloopmissingnsz test
llvm/lib/Analysis/IVDescriptors.cpp
597–599

Thanks for the suggestion, this is much easier to follow

spatel accepted this revision.Feb 12 2021, 10:46 AM

LGTM

This revision is now accepted and ready to land.Feb 12 2021, 10:46 AM