This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv Core 06/14] Do not fold constants on reading in IR asm/bitcode
AcceptedPublic

Authored by sdmitrouk on Oct 26 2015, 6:56 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitrouk updated this revision to Diff 38404.Oct 26 2015, 6:56 AM
sdmitrouk retitled this revision from to [FPEnv Core 06/14] Do not fold constants on reading in IR asm/bitcode .
sdmitrouk updated this object.
sdmitrouk added reviewers: hfinkel, mehdi_amini.
sdmitrouk set the repository for this revision to rL LLVM.
sdmitrouk added subscribers: llvm-commits, resistor, scanon.

Why are we making this assumption? It seems strange to assume that all constant expressions infer KeepExceptions and KeepRounding.

Also, shouldn't we be folding these to SNaN if KeepExceptions is true?

Why are we making this assumption? It seems strange to assume that all constant expressions infer KeepExceptions and KeepRounding.

Constant expressions do not have fast-math flags, the sole purpose of setting them here is to prevent any kind of folding. As there is no way to specify flags, assuming FPEnv access should work for all cases.

I don't think anything should be folded in any reader or writer. Without this patch <input> != write(read(<input>)) for constant expressions, which is a problem in case of preserving side-effects, but is also an odd behaviour in general.

Also, shouldn't we be folding these to SNaN if KeepExceptions is true?

If you're talking about examples in test, then probably not, they return infinity or a number, SNaN is not equivalent to those values.

Why are we making this assumption? It seems strange to assume that all constant expressions infer KeepExceptions and KeepRounding.

Constant expressions do not have fast-math flags, the sole purpose of setting them here is to prevent any kind of folding. As there is no way to specify flags, assuming FPEnv access should work for all cases.

That sounds like an oversight. We have overflow flags (nsw/nuw) for integer math.

I don't think anything should be folded in any reader or writer. Without this patch <input> != write(read(<input>)) for constant expressions, which is a problem in case of preserving side-effects, but is also an odd behaviour in general.

Also, shouldn't we be folding these to SNaN if KeepExceptions is true?

If you're talking about examples in test, then probably not, they return infinity or a number, SNaN is not equivalent to those values.

I am not, I am thinking about the general case. It sounds reasonable to transform floating-point divide by zero into SNaN when fp-exceptions are enabled.

That sounds like an oversight. We have overflow flags (nsw/nuw) for integer math.

Actually I asked about this last year, but it appeared that there is not much use of fast-math flags for constant expressions.
See it on the list.

I am not, I am thinking about the general case. It sounds reasonable to transform floating-point divide by zero into SNaN when fp-exceptions are enabled.

As it can yield infinity, I'm not sure whether SNaN is a good replacement for it. It would nice to do not fold anything in some case, maybe when both flags are given, not sure if "exceptions" or "rounding" is good choice on their own.

Why are we making this assumption? It seems strange to assume that all constant expressions infer KeepExceptions and KeepRounding.

Constant expressions do not have fast-math flags, the sole purpose of setting them here is to prevent any kind of folding. As there is no way to specify flags, assuming FPEnv access should work for all cases.

That sounds like an oversight. We have overflow flags (nsw/nuw) for integer math.

I don't think anything should be folded in any reader or writer. Without this patch <input> != write(read(<input>)) for constant expressions, which is a problem in case of preserving side-effects, but is also an odd behaviour in general.

Also, shouldn't we be folding these to SNaN if KeepExceptions is true?

If you're talking about examples in test, then probably not, they return infinity or a number, SNaN is not equivalent to those values.

I am not, I am thinking about the general case. It sounds reasonable to transform floating-point divide by zero into SNaN when fp-exceptions are enabled.

That sounds like an oversight. We have overflow flags (nsw/nuw) for integer math.

Actually I asked about this last year, but it appeared that there is not much use of fast-math flags for constant expressions.
See it on the list.

I am not, I am thinking about the general case. It sounds reasonable to transform floating-point divide by zero into SNaN when fp-exceptions are enabled.

As it can yield infinity, I'm not sure whether SNaN is a good replacement for it. It would nice to do not fold anything in some case, maybe when both flags are given, not sure if "exceptions" or "rounding" is good choice on their own.

I meant the more specific case where we are trying to fold 0.0/0.0.

It seems to me that there are three cases:

  • No flags on the constant expression: we can perform whatever folding we like
  • Exception flag is present: we need to fold to a new constant. Perhaps we could name it DynamicNaN. The job of DynamicNaN is to model the fact that expressions like 0.0/0.0 may or may not trap at runtime depending on how the program uses runtime calls like fegetexceptflag and fesetexceptflag.
  • Rounding flag is present: we cannot fold unless there is an unambiguous result.

That sounds like an oversight. We have overflow flags (nsw/nuw) for integer math.

Actually I asked about this last year, but it appeared that there is not much use of fast-math flags for constant expressions.
See it on the list.

I believe those concerns are primarily aimed at fast math flags which disable optimizations. On your other patch, I commented that we should rephrase the two flags you proposed as enabling, rather than disabling, optimizations.

sdmitrouk updated this revision to Diff 41914.Dec 4 2015, 12:06 PM

This got reduced to just a test case (maybe worth squashing into other revision, but this would break numbering, so still here).

mehdi_amini accepted this revision.Dec 11 2015, 3:57 PM
mehdi_amini edited edge metadata.
This revision is now accepted and ready to land.Dec 11 2015, 3:57 PM