[SimplifyCFG] Double check for constant expression domination

[sdmitrouk]

Summary

isSafeToSpeculativelyExecute() function contains more sophisticated checks
than Constant::canTrap(), so it appears to me that both functions should be
called to prevent optimizations that can harm.

I wasn't able to make a test, but absence of this check does cause trouble
with my local changes that disable certain kinds of optimizations to provide
IEEE754 conformant raising of FP exceptions, which I'd like to contribute.

[hfinkel]

This does not seem like the right fix. What is isSafeToSpeculativelyExecute checking that C->canTrap is not checking? I'd think that fixing C->canTrap is the right way to address this problem.

[nicholas]

[e-mail] wrote:

This does not seem like the right fix. What is isSafeToSpeculativelyExecute checking that C->canTrap is not checking? I'd think that fixing C->canTrap is the right way to address this problem.

I'd also like to make a plea for a testcase here. If we had a testcase,
we could see for ourselves what isSafeToSpeculativelyExecute is checking
that c->canTrap isn't.

http://reviews.llvm.org/D5459

---

llvm-commits mailing list
[e-mail]
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

[sdmitrouk]

Sorry for the delay.

This does not seem like the right fix. What is isSafeToSpeculativelyExecute checking that C->canTrap is not checking?

Looks like I misread the code of canTrap.

I'd think that fixing C->canTrap is the right way to address this problem.

It's not broken at the moment. It didn't work the way I'd expect after I modified isSafeToSpeculativelyExecute. If I get it right this time, I need to duplicate checks in isSafeToSpeculativelyExecute and canTrap, because the former one doesn't call the later one.

I'd also like to make a plea for a testcase here. If we had a testcase,
we could see for ourselves what isSafeToSpeculativelyExecute is checking
that c->canTrap isn't.

It works fine while these two functions are in sync, I missed that it's important to update both of them at the same time.

Thanks, now I understand that new checks should be introduced in a different way.

lib/Transforms/Utils/SimplifyCFG.cpp

static bool DominatesMergePoint(Value *V, BasicBlock *BB,
                                 SmallPtrSetImpl<Instruction*> *AggressiveInsts,
                                 unsigned &CostRemaining,
                                 const DataLayout *DL) {
   Instruction *I = dyn_cast<Instruction>(V);
   if (!I) {
     // Non-instructions all dominate instructions, but not all constantexprs
     // can be executed unconditionally.
     if (ConstantExpr *C = dyn_cast<ConstantExpr>(V))
-      if (C->canTrap())
+      if (C->canTrap() || !isSafeToSpeculativelyExecute(V, DL))
         return false;
     return true;
   }
   BasicBlock *PBB = I->getParent();
 
   // We don't want to allow weird loops that might have the "if condition" in
   // the bottom of this block.
   if (PBB == BB) return false;