This is an archive of the discontinued LLVM Phabricator instance.

ValueTracking: Implement computeKnownFPClass for sqrt
ClosedPublic

Authored by arsenm on Apr 8 2023, 7:51 PM.

Details

Summary

Could be slightly smarter in cases that are probably uninteresting.

Diff Detail

Event Timeline

arsenm created this revision.Apr 8 2023, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2023, 7:51 PM
arsenm requested review of this revision.Apr 8 2023, 7:51 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: wdng. · View Herald Transcript
arsenm updated this revision to Diff 512162.Apr 10 2023, 8:42 AM

Update unit test that was now too smart

llvm/lib/Analysis/ValueTracking.cpp
4723

This also includes -subnormal in DAZ=preserve-sign mode.

arsenm added inline comments.Apr 14 2023, 1:27 PM
llvm/lib/Analysis/ValueTracking.cpp
4723

The comment could elaborate on the preserve sign handling, but I think this is correct as is? There is a pre-existing bug in cannotBeOrderedLessThanZeroImpl for denormal handling

arsenm updated this revision to Diff 514013.Apr 16 2023, 7:59 AM

Fix DAZ handling

arsenm added a reviewer: foad.May 12 2023, 3:20 AM
foad added inline comments.May 12 2023, 4:05 AM
llvm/lib/Analysis/ValueTracking.cpp
4313

There's a better version of this in D148230 that uses inputDenormalIsIEEEOrPosZero.

4719

This should reuse info from the first call to computeKnownFPClass, instead of making a second call to cannotBeOrderedLessThanZeroImpl.

arsenm updated this revision to Diff 522529.May 16 2023, 4:00 AM
arsenm marked an inline comment as done.

Rebase to use new stuff

llvm/lib/Analysis/ValueTracking.cpp
4719

Oh right, this predates when I added cannotBeOrderedLessThanZero to KnownFPClass

foad added inline comments.May 16 2023, 4:20 AM
llvm/include/llvm/Analysis/ValueTracking.h
296

"known"

llvm/lib/Analysis/ValueTracking.cpp
4737–4738

Is this valid? You haven't implemented snan logic for most of the other intrinsics.

4742

Should really add OrderedLessThanZeroMask to the InterestedClasses for the recursive call for this case.

arsenm added inline comments.May 16 2023, 4:24 AM
llvm/lib/Analysis/ValueTracking.cpp
4737–4738

Yes, I need to go back to refine this. You are never allowed to introduce signaling nans. Non-constrained ops just don't guarantee quieting. The propagateNaN helper introduced later helps with this (although it doesn't quite apply here since there are additional nan conditions

foad accepted this revision.May 16 2023, 4:28 AM

LGTM with or without the InterestedClasses improvement.

This revision is now accepted and ready to land.May 16 2023, 4:28 AM
arsenm updated this revision to Diff 522541.May 16 2023, 4:34 AM

Interested classes

foad accepted this revision.May 16 2023, 4:35 AM