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

[sdmitrouk]

Summary

Modify ConstantExpression to store these flags and then use them.

[sdmitrouk]

Update of test per flags rename and correction of one condition (need to ignore non-floating-point types).

[sdmitrouk]

Use different overload of ConstantExpr::get().

[mehdi_amini]

include/llvm/IR/Constants.h

1080

Document the new parameter

[sdmitrouk]

Document the new parameter (Strict).

[majnemer]

lib/IR/Constants.cpp

1949–1950

Er, why not teach ConstantExpression about these flags?

[mehdi_amini]

(mark unaccepted waiting to settle the ConstantExpr case)

lib/IR/Constants.cpp

1949–1950

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?)?

[majnemer]

lib/IR/Constants.cpp

1949–1950

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.

[mehdi_amini]

lib/IR/Constants.cpp

1949–1950

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!

[sdmitrouk]

lib/IR/Constants.cpp

1949–1950

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;

[sdmitrouk]

Replacing with FastMathFlags for ConstantExpr, separate patch would almost completely undo this one and two more (two following ones, which are related to Strict flag), so I guess it's better to have it here.

[sdmitrouk]

Realized that I forgot to update other methods of ConstantExprKeyType to include the new field, will do that later, it won't change much. Optional data field was already used and it's only 7bits in Value class, which would need to be extended to hold new flags, so I added a field to ConstantExpr instead.

[mehdi_amini]

lib/AsmParser/LLParser.cpp

2985 ↗

(On Diff #43011)

Remove

lib/IR/ConstantsContext.h

423 ↗

(On Diff #43011)

Can we reuse one of the previous field?

[sdmitrouk]

Remove extra empty line.

[sdmitrouk]

lib/IR/ConstantsContext.h

423 ↗

(On Diff #43011)

In case of binary expressions SubclassData holds 0 and SubclassOptionalData holds binary operation specific flags. We can use SubclassData, which will be inconsistent with other types of constant expressions, but can be done if that's OK.

[mehdi_amini]

lib/Analysis/ConstantFolding.cpp

1007

Why not always use CE->getFastMathFlags();? Wasn't the FMF param added to this function because of the lack of FMF in ConstantExpr originally?

lib/IR/ConstantsContext.h

423 ↗

(On Diff #43352)

I think it is OK, unless you have any clue why it wouldn't?

[sdmitrouk]

lib/Analysis/ConstantFolding.cpp

1007

Wasn't the FMF param added to this function because of the lack of FMF in ConstantExpr originally?

No, it wasn't. FMF can be from instruction which contains CE as one of its operands, the argument was added for llvm::ConstantFoldInstruction.

lib/IR/ConstantsContext.h

423 ↗

(On Diff #43352)

No, I just didn't see such use and thought maybe there is a reason for that, which isn't documented. I'll change this then.

[mehdi_amini]

lib/Analysis/ConstantFolding.cpp

1007

Again, now that constant expression have dedicated FMF, I'm not sure why we should apply the parent instruction FMF?
If a constant expression does not have FMF enabled and it an operand of an instruction with FMF enabled it does not give license to fold I think.
And on the opposite, a constant expression with FMF as an operand of an instruction with FMF disabled should be foldable.

It is very possible that I miss something, David and/or Hal may have an opinion on this?

[sdmitrouk]

lib/Analysis/ConstantFolding.cpp

1007

Maybe we can drop it, I'll need to check (don't remember by now) and see whether it causes any problems. Initially it did make sense to propagate flags of instruction to its operands, I agree that it might be useless now.

[sdmitrouk]

Reuse SubclassData and remove FastMathFlags argument.

[mehdi_amini]

LGTM. Thanks.

include/llvm/IR/Constants.h

static Constant *getSelect(Constant *C, Constant *V1, Constant *V2,
                              Type *OnlyIfReducedTy = nullptr);
 
   /// get - Return a binary or shift operator constant expression,
   /// folding if possible.
   ///
   /// \param OnlyIfReducedTy see \a getWithOperands() docs.
   static Constant *get(unsigned Opcode, Constant *C1, Constant *C2,
                        unsigned Flags = 0, Type *OnlyIfReducedTy = nullptr,
-                       FastMathFlags FMF = FastMathFlags());
+                       FastMathFlags FMF = FastMathFlags(),
+                       bool Strict = false);
 
   /// \brief Return an ICmp or FCmp comparison operator constant expression.
   ///
   /// \param OnlyIfReduced see \a getWithOperands() docs.
   static Constant *getCompare(unsigned short pred, Constant *C1, Constant *C2,
                               bool OnlyIfReduced = false);
 
   /// get* - Return some common constants without having to

lib/Analysis/ConstantFolding.cpp

 /// using the specified DataLayout.  If successful, the constant result is
 /// result is returned, if not, null is returned.
 Constant *llvm::ConstantFoldConstantExpression(const ConstantExpr *CE,
                                                const DataLayout &DL,
                                                const TargetLibraryInfo *TLI,
                                                FastMathFlags FMF) {
   SmallPtrSet<ConstantExpr *, 4> FoldedOps;
   return ConstantFoldConstantExpressionImpl(CE, DL, TLI, FoldedOps, FMF);
 }
 
 /// Attempt to constant fold an instruction with the
 /// specified opcode and operands.  If successful, the constant result is
 /// returned, if not, null is returned.  Note that this function can fail when
 /// attempting to fold instructions like loads and stores, which have no
 /// constant expression form.
 ///
 /// TODO: This function neither utilizes nor preserves nsw/nuw/inbounds/etc
 /// information, due to only being passed an opcode and operands. Constant
 /// folding using this function strips this information.
 ///
 Constant *llvm::ConstantFoldInstOperands(unsigned Opcode, Type *DestTy,
                                          ArrayRef<Constant *> Ops,
                                          const DataLayout &DL,
                                          const TargetLibraryInfo *TLI,
                                          FastMathFlags FMF) {
   // Handle easy binops first.
   if (Instruction::isBinaryOp(Opcode)) {
     if (isa<ConstantExpr>(Ops[0]) || isa<ConstantExpr>(Ops[1])) {
       if (Constant *C = SymbolicallyEvaluateBinop(Opcode, Ops[0], Ops[1], DL))
         return C;
     }
 
-    return ConstantExpr::get(Opcode, Ops[0], Ops[1], 0, nullptr, FMF);
+    return ConstantExpr::get(Opcode, Ops[0], Ops[1], 0, nullptr,
+                             FMF, /* Strict = */ true);
   }
 
   switch (Opcode) {
   default: return nullptr;
   case Instruction::ICmp:
   case Instruction::FCmp: llvm_unreachable("Invalid for compares");
   case Instruction::Call:
     if (Function *F = dyn_cast<Function>(Ops.back()))

lib/IR/Constants.cpp

if (SrcScalarTy->getElementType() != DstElemTy) {
     }
     C = getBitCast(C, MidTy);
   }
   return getFoldedCast(Instruction::AddrSpaceCast, C, DstTy, OnlyIfReduced);
 }
 
 Constant *ConstantExpr::get(unsigned Opcode, Constant *C1, Constant *C2,
                             unsigned Flags, Type *OnlyIfReducedTy,
-                            FastMathFlags FMF) {
+                            FastMathFlags FMF,
+                            bool Strict) {
   // Check the operands for consistency first.
   assert(Opcode >= Instruction::BinaryOpsBegin &&
          Opcode <  Instruction::BinaryOpsEnd   &&
          "Invalid opcode in binary constant expression");
   assert(C1->getType() == C2->getType() &&
          "Operand types in binary constant expression should match");
 
 #ifndef NDEBUG
#endif
 
   if (Constant *FC = ConstantFoldBinaryInstruction(Opcode, C1, C2, FMF)) {
     return FC;          // Fold a few common cases.
   }
 
   if (OnlyIfReducedTy == C1->getType())
     return nullptr;
 
+  // ConstantExpression can't store these flags, which can lead to going against
+  // them.
+  if (Strict && (FMF.keepExceptions() || FMF.keepRounding()))
+    return nullptr;
+
   Constant *ArgVec[] = { C1, C2 };
   ConstantExprKeyType Key(Opcode, ArgVec, 0, Flags);
 
   LLVMContextImpl *pImpl = C1->getContext().pImpl;
   return pImpl->ExprConstants.getOrCreate(C1->getType(), Key);
 }
 
 Constant *ConstantExpr::getSizeOf(Type* Ty) {

test/Transforms/EarlyCSE/fpenv.ll

+; %val.true and %val.false should not be moved inside phi-node as constant
+; expressions as such transformation looses fast-math flags.
+; RUN: opt < %s -S -early-cse | FileCheck %s
+
+define double @do-not-turn-into-constexpr(double %x) {
+; CHECK-LABEL: @do-not-turn-into-constexpr(
+; CHECK: %val.true = fmul
+; CHECK: %val.false = fmul
+entry:
+  %cmp = fcmp oeq double %x, 0.000000e+00
+  br i1 %cmp, label %cond.true, label %cond.false
+
+cond.true:                                        ; preds = %entry
+  %val.true = fmul kexc double 1.000000e+300, 1.000000e+300
+  br label %cond.end
+
+cond.false:                                       ; preds = %entry
+  %val.false = fmul kexc double 1.000000e-300, 1.000000e-300
+  br label %cond.end
+
+cond.end:                                         ; preds = %cond.false, %cond.true
+  %cond = phi double [ %val.true, %cond.true ], [ %val.false, %cond.false ]
+  ret double %cond
+}