[ValueTracking] Teach isSafeToSpeculativelyExecute() about the speculatable attribute

[arsenm]

Summary

Depnds on D20116

[• tstellarAMD]

I am planning to add the speculatable attribute to all the intrinsics listed in this function in a separate patch.

Do we want to also change this function to give information about speculatable cost as proposed in this review: https://reviews.llvm.org/D20116

Another question I have is should the presence of speculatable by itself be enough for isSafeToSpeculativelyExecute() to return true. This would mean that the callers would be responsible for checking memory dependencies before speculating. The alternative would be to make isSafeToSpeculativelyExecute() require both the readnone and speculatable attributes.

[arsenm]

Doesn't the SpeculativeExecution already considered the TTI cost for this anyway?

lib/Analysis/ValueTracking.cpp

3120–3125

Is invoke allowed to be speculatable?

[hfinkel]

In D22413#485588, @arsenm wrote:

Doesn't the SpeculativeExecution already considered the TTI cost for this anyway?

SimplifyCFG does, so we seem to be okay there.

This LGTM.

lib/Analysis/ValueTracking.cpp

3120–3125

No. The invoked function might throw. If it can't throw, then we'll convert it into a regular call during simplification.

[• tstellarAMD]

lib/Analysis/ValueTracking.cpp

3120–3125

So do I need to check for invoke here, or is it safe to assume that functions referenced by invoke will never have the speculatable attribute.?

[hfinkel]

lib/Analysis/ValueTracking.cpp

3120–3125

No, I don't think so.

[arsenm]

lib/Analysis/ValueTracking.cpp

3120–3125

A verifier check should probably be added in the other patch for this if it isn't already there

[majnemer]

lib/Analysis/ValueTracking.cpp

3121

I'd do auto * here.

3125–3128

Can a FIXME get added to mark these as speculatable in the tablegen file?

[hfinkel]

lib/Analysis/ValueTracking.cpp

3120–3125

Do you mean a verifier check that a function that might throw is marked as speculatable? I'm not sure that's a good idea.

[• tstellarAMD]

Address some review comments.

[arsenm]

lib/Analysis/ValueTracking.cpp

3120–3125

I mean the opposite, that an invoke never has a speculatable

[majnemer]

lib/Analysis/ValueTracking.cpp

3120–3125

IMO, it seems like it would belong more in llvm's Lint analysis than a verifier check.

[hfinkel]

lib/Analysis/ValueTracking.cpp

3120–3125

I agree. We could easily end up with invoked functions being speculatable as an intermediate state (especially if we start inferring the attribute).

Also, it is not clearly a logical contradiction. The user might add the speculatable attribute on a function that might throw because the user does not care if the exception is thrown early. I'm not sure what we would do with that, however, because moving the catch blocks around to speculate the invoke seems unlikely worthwhile.

[arsenm]

r301688

lib/Analysis/ValueTracking.cpp

if (!LI->isUnordered() ||
         // Speculative load may load data from dirty regions.
         LI->getParent()->getParent()->hasFnAttribute(
             Attribute::SanitizeAddress))
       return false;
     const DataLayout &DL = LI->getModule()->getDataLayout();
     return isDereferenceableAndAlignedPointer(LI->getPointerOperand(),
                                               LI->getAlignment(), DL, CtxI, DT);
   }
   case Instruction::Call: {
+    auto *CI = cast<const CallInst>(Inst);
+    const Function *Callee = CI->getCalledFunction();
+    if (Callee && Callee->isSpeculatable())
+      return true;
     if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
       switch (II->getIntrinsicID()) {
       // These synthetic intrinsics have no side-effects and just mark
       // information about their operands.
       // FIXME: There are other no-op synthetic instructions that potentially
       // should be considered at least *safe* to speculate...
+      // FIXME: The speculatable attribute should be added to all these
+      // intrinsics and this case statement should be removed.
       case Intrinsic::dbg_declare:
       case Intrinsic::dbg_value:
         return true;
 
       case Intrinsic::bswap:
       case Intrinsic::ctlz:
       case Intrinsic::ctpop:
       case Intrinsic::cttz:

test/Transforms/SimplifyCFG/speculate-call.ll

+; RUN: opt -S -simplifycfg < %s | FileCheck %s
+
+; CHECK-LABEL: @speculatable_attribute
+; CHECK: select
+define i32 @speculatable_attribute(i32 %a) {
+entry:
+  %c = icmp sgt i32 %a, 64
+  br i1 %c, label %end, label %if
+
+if:
+  %val = call i32 @func() #0
+  br label %end
+
+end:
+  %ret = phi i32 [%val, %if], [0, %entry]
+  ret i32 %ret
+}
+
+define i32 @func() #0 {
+  ret i32 1
+}
+attributes #0 = { nounwind readnone speculatable }
+