[llvm-commits] [llvm] r147036 - in /llvm/trunk: lib/Analysis/ValueTracking.cpp lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/SpeculativeExec.ll

[Nick Lewycky]

On 12/21/2011 11:02 AM, Dan Gohman wrote:
>
> On Dec 20, 2011, at 9:52 PM, Nick Lewycky wrote:
>
>> Author: nicholas
>> Date: Tue Dec 20 23:52:02 2011
>> New Revision: 147036
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=147036&view=rev
>> Log:
>> Make some intrinsics safe to speculatively execute.
>>
>> Modified:
>>     llvm/trunk/lib/Analysis/ValueTracking.cpp
>>     llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>>     llvm/trunk/test/Transforms/SimplifyCFG/SpeculativeExec.ll
>>
>> Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=147036&r1=147035&r2=147036&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
>> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Tue Dec 20 23:52:02 2011
>> @@ -1912,11 +1912,31 @@
>>        return false;
>>      return LI->getPointerOperand()->isDereferenceablePointer();
>>    }
>> -  case Instruction::Call:
>> +  case Instruction::Call: {
>> +   if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
>> +     switch (II->getIntrinsicID()) {
>> +       case Intrinsic::bswap:
>> +       case Intrinsic::ctlz:
>> +       case Intrinsic::ctpop:
>> +       case Intrinsic::cttz:
>> +       case Intrinsic::objectsize:
>> +       case Intrinsic::sadd_with_overflow:
>> +       case Intrinsic::smul_with_overflow:
>> +       case Intrinsic::ssub_with_overflow:
>> +       case Intrinsic::uadd_with_overflow:
>> +       case Intrinsic::umul_with_overflow:
>> +       case Intrinsic::usub_with_overflow:
>> +         return true;
>> +       // TODO: some fp intrinsics are marked as having the same error handling
>> +       // as libm. They're safe to speculate when they won't error.
>> +       // TODO: are convert_{from,to}_fp16 safe?
>> +       // TODO: can we list target-specific intrinsics here?
>
> Just checking mayHaveSideEffects() should cover almost everything here.

Good point! Is there anything that mayHaveSideEffects() would return 
true on that wouldn't be safe to speculate? The comment on 
mayHaveSideEffects claims that a call to malloc would return false, but 
that's a lie, suggesting that an audit of users of these two functions 
is due...

>
>> +       default: break;
>> +     }
>> +   }
>>      return false; // The called function could have undefined behavior or
>> -                  // side-effects.
>> -                  // FIXME: We should special-case some intrinsics (bswap,
>> -                  // overflow-checking arithmetic, etc.)
>> +                  // side-effects, even if marked readnone nounwind.
>> +  }
>>    case Instruction::VAArg:
>>    case Instruction::Alloca:
>>    case Instruction::Invoke:
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=147036&r1=147035&r2=147036&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Tue Dec 20 23:52:02 2011
>> @@ -293,6 +293,7 @@
>>      Cost = 1;
>>      break;   // These are all cheap and non-trapping instructions.
>>
>> +  case Instruction::Call:
>
> cttz, ctlz, etc. are very expensive on many targets. Having SimplifyCFG
> speculate them here could cause substantial pessimizations.

Yes, and indeed it won't because the cost of these is 2 and the 
threshold is 1. That's why the test needs -phi-node-folding-threshold=2 
to work.

Ultimately the IR-level optimizers will always do better with a select 
instead of branch+phi, and I'd like to instead teach codegen to turn 
selects into branches (sinking the computations on each side) when 
profitable.

Nick