SpeculativeExecution: Stop using whitelist for costs

[arsenm]

Summary

Just let TTI's cost do this instead of arbitrarily restricting this.

[arsenm]

Attach correct version

[reames]

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Assuming you've done the above, LGTM.

[arsenm]

In D24544#546434, @reames wrote:

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Assuming you've done the above, LGTM.

I only noticed what D24542 fixes, but it's possible I missed something else

[arsenm]

Committed a halfway step in r285438 until I get a chance to better check the set of operations and fix the vector indexing

[arsenm]

r301950

[chandlerc]

In D24544#546434, @reames wrote:

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Perhaps unsurprisingly, this review comment proved prophetic.

This patch causes us to "miscompile" some CUDA kernels.

That said, LLVM may not be at fault as PTXAS has known bugs that miscompile valid PTX programs, so I'm going to try to somehow produce a test case that shows what is going on here.

In the meantime, it sure would be nice to do as Philip suggested and make that switch covering so that we don't just have the default be safe. Or at least make the default be unsafe, which seems ... far better.

[chandlerc]

In D24544#750762, @chandlerc wrote:

In D24544#546434, @reames wrote:

I'd suggest looking over the isSafeToSpeculate code carefully. If we had any bugs there, this whitelist might have been helping to cover over them in practice. In particular, the default: return true; case in that function looks suspicious. You might want to convert that into a fully covered switch and cross reference the two lists to make sure this change isn't accidentally semantic.

Perhaps unsurprisingly, this review comment proved prophetic.

This patch causes us to "miscompile" some CUDA kernels.

That said, LLVM may not be at fault as PTXAS has known bugs that miscompile valid PTX programs, so I'm going to try to somehow produce a test case that shows what is going on here.

After some basic investigation, this patch is really broken.

isSafeToSpeculativelyExecute doesn't actually mean it is safe to hoist something. You need a lot more to guarantee that:

• Does it read from memory?
  • Then you can't hoist it over a clobbering store
  • You can't hoist it over a function call which could map that memory
  • You can't hoist it over a synchronization point with another thread that could map that memory (or be a clobbering store)

I haven't even thought hard about what the full, safe predicate is here. But I suspect this pass needs to look very closely at what existing, well tested LLVM passes use to do hoisting, or it needs to continue to use a whitelist.

For now, I'm reverting this patch because I don't even know how many test cases need to be written here.

Also, if the whitelist is removed, the test against UINT_MAX shoudl be as well.

In the meantime, it sure would be nice to do as Philip suggested and make that switch covering so that we don't just have the default be safe. Or at least make the default be unsafe, which seems ... far better.

And we should still do this as I don't really have a lot of confidence in that routine as-is.

[uabelho]

This also caused https://bugs.llvm.org//show_bug.cgi?id=32964

I set that PR to resolved now since the change was reverted, but there is a somewhat small ll file
that reproduces the problem.

lib/Transforms/Scalar/SpeculativeExecution.cpp

if (Succ1.size() == 1) // equivalent to if-then
       return considerHoistingFromTo(Succ0, B);
     if (Succ0.size() == 1) // equivalent to if-else
       return considerHoistingFromTo(Succ1, B);
   }
 
   return false;
 }
 
-static unsigned ComputeSpeculationCost(const Instruction *I,
-                                       const TargetTransformInfo &TTI) {
-  switch (Operator::getOpcode(I)) {
-    case Instruction::GetElementPtr:
-    case Instruction::Add:
-    case Instruction::Mul:
-    case Instruction::And:
-    case Instruction::Or:
-    case Instruction::Select:
-    case Instruction::Shl:
-    case Instruction::Sub:
-    case Instruction::LShr:
-    case Instruction::AShr:
-    case Instruction::Xor:
-    case Instruction::ZExt:
-    case Instruction::SExt:
-    case Instruction::Call:
-    case Instruction::BitCast:
-    case Instruction::PtrToInt:
-    case Instruction::IntToPtr:
-    case Instruction::AddrSpaceCast:
-    case Instruction::FPToUI:
-    case Instruction::FPToSI:
-    case Instruction::UIToFP:
-    case Instruction::SIToFP:
-    case Instruction::FPExt:
-    case Instruction::FPTrunc:
-    case Instruction::FAdd:
-    case Instruction::FSub:
-    case Instruction::FMul:
-    case Instruction::FDiv:
-    case Instruction::FRem:
-    case Instruction::ICmp:
-    case Instruction::FCmp:
-    case Instruction::ExtractElement:
-    case Instruction::InsertElement:
-    case Instruction::ShuffleVector:
-    case Instruction::ExtractValue:
-    case Instruction::InsertValue:
-      return TTI.getUserCost(I);
-
-    default:
-      return UINT_MAX; // Disallow anything not whitelisted.
-  }
-}
-
 bool SpeculativeExecutionPass::considerHoistingFromTo(
     BasicBlock &FromBlock, BasicBlock &ToBlock) {
   SmallSet<const Instruction *, 8> NotHoisted;
   const auto AllPrecedingUsesFromBlockHoisted = [&NotHoisted](User *U) {
     for (Value* V : U->operand_values()) {
       if (Instruction *I = dyn_cast<Instruction>(V)) {
         if (NotHoisted.count(I) > 0)
           return false;
       }
     }
     return true;
   };
 
   unsigned TotalSpeculationCost = 0;
   for (auto& I : FromBlock) {
-    const unsigned Cost = ComputeSpeculationCost(&I, *TTI);
+    const unsigned Cost = TTI->getUserCost(&I);
     if (Cost != UINT_MAX && isSafeToSpeculativelyExecute(&I) &&
         AllPrecedingUsesFromBlockHoisted(&I)) {
       TotalSpeculationCost += Cost;
       if (TotalSpeculationCost > SpecExecMaxSpeculationCost)
         return false;  // too much to hoist
     } else {
       NotHoisted.insert(&I);
       if (NotHoisted.size() > SpecExecMaxNotHoisted)