This is an archive of the discontinued LLVM Phabricator instance.

InstSimplify: Simplifications for ldexp
ClosedPublic

Authored by arsenm on May 1 2023, 7:36 AM.

Details

Summary

Ported from old amdgcn intrinsic which will soon be deleted.

Diff Detail

Event Timeline

arsenm created this revision.May 1 2023, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2023, 7:36 AM
arsenm requested review of this revision.May 1 2023, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2023, 7:36 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad added inline comments.May 2 2023, 3:01 AM
llvm/lib/Analysis/ConstantFolding.cpp
1578

Should keep support for the old intrinsic while it still exists.

2682

Should keep support for the old intrinsic while it still exists.

foad added inline comments.May 2 2023, 3:06 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6128

Why is this not strictfp-safe?

foad added a comment.May 2 2023, 3:21 AM

I think you can simplify ldexp(x, C) -> x * ldexp(1.0, C). Even for strictfp this should work if you use a constrained fmul and the if the new ldexp itself does not overflow or underflow.

kpn added inline comments.May 2 2023, 5:27 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6128

It would pass through an SNaN instead of quieting it I expect.

arsenm added inline comments.May 2 2023, 6:47 PM
llvm/lib/Analysis/ConstantFolding.cpp
2682

There's no reason to aim for performance compatibility with something trivially replaceable. It's already dropped in D149589

llvm/lib/Analysis/InstructionSimplify.cpp
6128

Yes, needs to quiet/canonicalize which isn't guaranteed for non-constrained ops

foad added inline comments.Jul 11 2023, 2:42 AM
llvm/lib/Analysis/ConstantFolding.cpp
2682

This is dead code with the extra case you added above.

llvm/lib/Analysis/InstructionSimplify.cpp
6104

Why is this not strictfp-safe?

6126

Why is this not strictfp-safe?

Maybe I just need a good description of what strictfp implies. The description in the langref mentions rounding mode, status flags and trapping, but says nothing about quieting NaNs.

arsenm added inline comments.Jul 11 2023, 3:54 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

If undef resolved to a signaling nan it wouldn't raise an exception

6126

A signaling nan is supposed to raise an exception which quieting it would hide.

The LangRef states signaling nans may not be quieted by non-constrained operations and constrained should handle them properly

foad added inline comments.Jul 11 2023, 4:00 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

But here you are choosing what you want the undef value to be, so choose a quiet NaN.

arsenm added inline comments.Jul 11 2023, 4:15 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104
arsenm added inline comments.Jul 11 2023, 4:17 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

although really I should probably just call simplifyFPOp in the first place

foad added inline comments.Jul 11 2023, 4:46 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

https://github.com/llvm/llvm-project/blob/67a212af4c24426de6e436e9b82590d41faa665c/llvm/lib/Analysis/InstructionSimplify.cpp#L5513

Well I don't understand why that code doesn't propagate quiet NaNs unconditionally. I agree with @sepavloff's comment: https://reviews.llvm.org/D103169#inline-979968

Also, that code handles all fp ops but here we only care specifically about ldexp. Where is the spec for what fp exceptions ldexp can raise? man ldexp mentions exceptions on overflow and underflow, but does not mention raising invalid operation even on a signalling NaN input.

In any case a comment explaining why each case is supposedly not fpstrict-safe would really help, since this stuff is massively non-obvious.

arsenm added inline comments.Jul 11 2023, 4:48 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

man ldexp says:

Range error, overflow
       errno is set to ERANGE.  An overflow floating-point exception (FE_OVERFLOW) is raised.

Range error, underflow
       errno is set to ERANGE.  An underflow floating-point exception (FE_UNDERFLOW) is raised.
6104

but does not mention raising invalid operation even on a signalling NaN input.

This is implied for every FP operation. There are just the exceptions for fabs/fneg/copysign/is.fpclass

kpn added inline comments.Jul 11 2023, 6:28 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6126

With "maytrap' we are allowed to remove exceptions. That should make quieting an sNaN safe, no? Also, are callers checking for the default fp environment? That should behave the same as non-constrained operations.

arsenm added inline comments.Jul 11 2023, 6:54 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6126

I just remembered this also has the problem that the constrained operations aren't fully expressive enough. "Default FP environment" doesn't cover denormal flushing or other target dependent modes.

arsenm updated this revision to Diff 539092.Jul 11 2023, 7:30 AM

Drop dead code, add strictfp and FMF todo. I looked into merging with simplifyFPOp, but it would multiply the complexity of the patch. I also don't necessarily agree we have adequate information to fold these given denormal input exceptions exist and the denormal mode is dynamically changeable

foad added inline comments.Jul 11 2023, 8:56 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6120

Also handle qNaN here?

arsenm added inline comments.Jul 11 2023, 9:20 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6120

Technically would add more brokenness to old mips signaling nans although I don’t think we have a ruling on how much we should care

kpn added inline comments.Jul 12 2023, 10:34 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6120

Isn't this a problem that should be solved in APFloat? Anyway, it seems like we shouldn't let old mips keep us from optimizing in the present.

arsenm added inline comments.Jul 12 2023, 11:34 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6120

APFloat would need to know what to do and maybe treat it as a separate type. Maybe it should be part of DataLayout, I don’t know

The point of the patch is to optimize the regular version. I handled the strictfp parts that do not require any thought. I don't want to further complicate this step for strictfp

ping, this should be in the release that introduced the intrinsic

llvm/lib/Analysis/InstructionSimplify.cpp
6104

I think for this block of code, just deferring to simplifyFPOp is better; the only thing that differs here is that ldexp(x, undef) needs to fold to x.

6132

Rounding mode doesn't come into play, since ldexp is always an exact operation.

arsenm added inline comments.Jul 21 2023, 1:26 PM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

simplifyFPOp complicates things because that's expecting only float inputs and here there's an integer op

arsenm updated this revision to Diff 545276.Jul 28 2023, 2:10 PM
arsenm marked an inline comment as done.

Fix comment

llvm/lib/Analysis/InstructionSimplify.cpp
6104

I keep trying to make it use simplifyFPOp and I'm unhappy with it. It's ignoring the denorm flushing potential, uses FastMathFlags and i'm about 80% sure the precedent here for getting to the FMF is broken. I see various places using the CxtI in the SimplifyQuery, which is likely not the instruction the flags are actually attached to. It's not really less code to just handle the nan case directly while I have the APFloat

foad added inline comments.Aug 1 2023, 2:33 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

I've read all the comments here and I still don't understand why this case is not strictfp-safe.

arsenm added inline comments.Aug 1 2023, 2:42 PM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

Folding to the input operand drops a canonicalize. It would be more correct to introduce llvm.experimental.constrained.canonicalize which does not exist

foad added inline comments.Aug 2 2023, 12:54 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

You have a free choice of what input operand value to assume, so choose one for which llvm.experimental.constrained.canonicalize would be a no-op? Isn't that already true for the value returned by getNaN? (Or is this some MIPS weirdness again where we don't know whether that NaN is quiet or not?)

arsenm added inline comments.Aug 2 2023, 4:19 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6104

Maybe for the ldexp(undef, x) -> nan case (where we evidently don't have concrete rules for payload bits)

I'm talking about the ldexp(x, undef) -> x case

arsenm added a comment.Aug 7 2023, 7:40 AM

ping, I'll drop strictfp support completely and never come back to it if it moves this along

foad added a comment.Aug 10 2023, 5:28 AM

I still think removing support for amdgcn_ldexp should be in the patch that removes amdgcn_ldexp.

I still think every simplification guarded by !IsStrict should have a comment saying why, even if it's just "to be conservative because we're not *sure* that it's fpstrict-safe".

arsenm updated this revision to Diff 549485.Aug 11 2023, 12:10 PM
foad added inline comments.Aug 15 2023, 2:28 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6103

I still don't understand why this one isn't strictfp-safe, if you simplify -> qnan.

arsenm added inline comments.Aug 15 2023, 12:51 PM
llvm/lib/Analysis/InstructionSimplify.cpp
6103

if undef could be anything, it could have been a signaling nan that would demand quieting

arsenm added inline comments.Aug 15 2023, 12:52 PM
llvm/lib/Analysis/InstructionSimplify.cpp
6103

Plus we evidently don't have agreement on how nan payload bits are supposed to work

foad added inline comments.Aug 16 2023, 3:58 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6103

it could have been a signaling nan

That's like saying "it could have been 99.9". The point is we are *choosing* a particular value to refine it to, so why not choose a quiet nan?

Plus we evidently don't have agreement on how nan payload bits are supposed to work

OK, if we are unable to make a quiet nan because we don't know what bits to put in the payload, that seems like a good reason - but please add a comment to that effect since it is massively non-obvious. (Also if that is true then how does C->makeQuiet() below work??)

arsenm added inline comments.Aug 16 2023, 5:42 AM
llvm/lib/Analysis/InstructionSimplify.cpp
6103

Make quiet just flips the bit of an existing nan which is obviously ok (ignoring old mips). This is synthesizing a new choice

arsenm updated this revision to Diff 550739.Aug 16 2023, 7:01 AM

Just fold the undef for strict, this is near universally broken anyway if it's decided it's broken

jcranmer-intel accepted this revision.Sep 12 2023, 1:16 PM
This revision is now accepted and ready to land.Sep 12 2023, 1:16 PM