[PATCH] D72466: [FPEnv][RFC] Invert sense of MIFlag::FPExcept flag

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand marked an inline comment as done.
uweigand added inline comments.


================
Comment at: llvm/test/CodeGen/X86/fp-intrinsics-flags.ll:9
 ; CHECK: MOVSDmr %stack.0, 1, $noreg, 0, $noreg, killed [[CVTSI2SDrr]] :: (store 8 into %stack.0, align 4)
-; CHECK: [[LD_Fp64m80_:%[0-9]+]]:rfp80 = LD_Fp64m80 %stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw :: (load 8 from %stack.0, align 4)
+; CHECK: [[LD_Fp64m80_:%[0-9]+]]:rfp80 = nofpexcept LD_Fp64m80 %stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw :: (load 8 from %stack.0, align 4)
 ; CHECK: RET 0, killed [[LD_Fp64m80_]]
----------------
pengfei wrote:
> craig.topper wrote:
> > pengfei wrote:
> > > craig.topper wrote:
> > > > pengfei wrote:
> > > > > pengfei wrote:
> > > > > > craig.topper wrote:
> > > > > > > I don't know how to fix this. This is a pattern match from a any extending load instruction. There's no flag to copy from. Unfortunately, X87 generates an exception if you load a NAN from a float or double memory location. But I don't know how to represent that in SelectionDAG or IR.
> > > > > > SSE instructions don't generate exceptions when only loading from memory to register. Maybe other targets too. And current strict FP semantics don't define a strict load. Can we add masking the `#I` before FLD and FPCW recovery operations into the pattern?
> > > > > Oh, we also add those instructions in non-strict scenarios. So it's not practicable.
> > > > What happens to the SNAN or denormal value that was loaded if the exception is masked. Does the SNAN get quieted or does it stay an SNAN in 80-bit format? If masking quiets it then the SNAN would never signal. For SSE it would be signalled when the SNAN is operated on by an arithmetic instruction.
> > > sNaN will be kept sNaN in 80-bit while qNaN to be qNaN, tested by below code:
> > > ```
> > > void foo(unsigned a) {
> > >   fedisableexcept(FE_INVALID);
> > >   asm("fld1\nflds %0\nfwait" :: "m"(a));
> > >   feenableexcept(FE_INVALID);
> > >   asm("fucom\nfwait");
> > > }
> > > ```
> > > 
> > > But it becomes more complicated for denormal. Because any denormal in float and double will become normal value in 80-bit format. So we should keep `#D` unchanged, which means we may still raise exception when loading memory in X87, and we can't keep the same behavior with SSE.
> > For the SNAN test, you need to clear the exception status bits before unmasking the exception. Otherwise the exception is still pending and will be taken when its unmasked.
> You are correct! They all turn to qNaN after fld. The former exception came from the pending exception.
The test case starts out as just a plain @llvm.experimental.constrained.sitofp.f64.i8
This gets translated at the start of isel to
        t3: i8,ch = load<(load 1 from %fixed-stack.0)> t0, FrameIndex:i32<-1>, undef:i32
      t4: f64,ch = strict_sint_to_fp t0, t3
    t6: f80 = fp_extend t4

The strict_sint_to_fp then becomes a CVTSI2SDrr (correctly marked as raising exceptions), while the fp_extend becomes a combination of MOVSDmr and LD_Fp64m80.

It seems to me the problem originates with the fp_extend -- which already should be a strict_fp_extend, really.  Then the strict_fp_extend should be converted to a series of fpexcept MI instructions.

I'm not sure exactly where the fp_extend comes from, but I'd assume this is a conversion mandated by the ABI?  In that case, it might make sense to check whether the function is marked with the strictfp attribute and generate strict conversions by the ABI interface code in that case.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72466/new/

https://reviews.llvm.org/D72466