[PATCH] D14073: [FPEnv Core 08/14] Do not simplify expressions with FPEnv access

[Sergey Dmitrouk via llvm-commits]

sdmitrouk added inline comments.

================
Comment at: lib/IR/Constants.cpp:1890-1891
@@ +1889,4 @@
+  if (C1->getType()->isFPOrFPVectorTy()) {
+    // ConstantExpression can't store these flags, which can lead to going
+    // against them.
+    if (Strict && (!FMF.noExceptions() || !FMF.noRounding()))
----------------
joker.eph wrote:
> majnemer wrote:
> > joker.eph wrote:
> > > majnemer wrote:
> > > > Er, why not teach ConstantExpression about these flags?
> > > I thought that since it was float specific it couldn't be added, but I just checked and saw that we have integer nsw/nuw already on ConstantExpr, so it seems possible.
> > > 
> > > `ConstantExprKeyType` accepts `unsigned short SubclassOptionalData`. FastMathFlags are currently `unsigned Flags` but there is not reason they couldn't be `uint16_t` instead right?
> > > 
> > > David: do you know why we don't already have FastMath on ConstantExpr? Is it because there was no case with FP where we wouldn't constant fold immediately?
> > > 
> > > Now, what are the potential benefit to deal with Floating point ConstantExpr with FastMath flags? And what are the complications (i.e. will we have to add extra care everywhere we deal with FP ConstantExpr?)?
> > AFAIK, yes.  We didn't believe the flags would meaningfully change the result of what expression we'd give back.  This would change with the new flags.
> Talk with David on IRC, I agree with him that it seems cleaner to have this.
> Sergey: can you prepare a separate patch that would add FMF to ConstantExpr?
> Thanks!
I forgot about this change and another code related to constant expressions got removed from different patch, so I thought there is no need to do something about them.

> can you prepare a separate patch that would add FMF to ConstantExpr?

I can try if it's just adding the field and considering it in several places where folding is performed. If it's significantly more complex, then I'm afraid someone else should do this as I'll stop contributing to LLVM very soon (and either upload the rest of the patches for ARM and FENV pragma in Clang or pass them over to somebody else to do this).

By the way, the field is not of `unsigned short` type:
```
  uint8_t SubclassOptionalData;
```


Repository:
  rL LLVM

http://reviews.llvm.org/D14073