[llvm] r230775 - Change the fast-isel-abort option from bool to int to enable "levels"

[Mehdi Amini]

> On Mar 3, 2015, at 5:16 AM, Andrea Di Biagio <andrea.dibiagio at gmail.com> wrote:
> 
> Hi Mehdi,
> 
> At the moment, if fast-isel aborts and flag fast-isel-abort is set, we
> end up calling 'llvm_unreachable'.
> Wouldn't it be better to use 'report_fatal_error' instead of calling
> 'llvm_unreachable’?

That sounds reasonable.

r231201

Thanks,

Mehdi





> 
> Example:
> 
>> <snip>...
>>   const Function &Fn = *mf.getFunction();
>>   MF = &mf;
>> @@ -1182,7 +1181,7 @@ void SelectionDAGISel::SelectAllBasicBlo
>>         if (!FastIS->lowerArguments()) {
>>           // Fast isel failed to lower these arguments
>>           ++NumFastIselFailLowerArguments;
>> -          if (EnableFastISelAbortArgs)
>> +          if (EnableFastISelAbort > 1)
>>             llvm_unreachable("FastISel didn't lower all arguments");
> 
> 
> If we use 'report_fatal_error', then we avoid to get an 'UNREACHABLE
> executed' message if EnableFastISelAbort is > 1.
> 
>> 
>>           // Use SelectionDAG argument lowering
>> @@ -1252,6 +1251,10 @@ void SelectionDAGISel::SelectAllBasicBlo
>>             dbgs() << "FastISel missed call: ";
>>             Inst->dump();
>>           }
>> +          if (EnableFastISelAbort > 2)
>> +            // FastISel selector couldn't handle something and bailed.
>> +            // For the purpose of debugging, just abort.
>> +            llvm_unreachable("FastISel didn't select the entire block");
> 
> Same here. Wouldn't it be better to use 'report_fatal_error' ?
> 
>> 
>>           if (!Inst->getType()->isVoidTy() && !Inst->use_empty()) {
>>             unsigned &R = FuncInfo->ValueMap[Inst];
>> @@ -1279,24 +1282,21 @@ void SelectionDAGISel::SelectAllBasicBlo
>>           continue;
>>         }
>> 
>> -        if (isa<TerminatorInst>(Inst) && !isa<BranchInst>(Inst)) {
>> -          // Don't abort, and use a different message for terminator misses.
>> -          NumFastIselFailures += NumFastIselRemaining;
>> -          if (EnableFastISelVerbose || EnableFastISelAbort) {
>> +        if (EnableFastISelVerbose || EnableFastISelAbort) {
>> +          if (isa<TerminatorInst>(Inst)) {
>> +            // Use a different message for terminator misses.
>>             dbgs() << "FastISel missed terminator: ";
>> -            Inst->dump();
>> -          }
>> -        } else {
>> -          NumFastIselFailures += NumFastIselRemaining;
>> -          if (EnableFastISelVerbose || EnableFastISelAbort) {
>> +          } else {
>>             dbgs() << "FastISel miss: ";
>> -            Inst->dump();
>>           }
>> -          if (EnableFastISelAbort)
>> -            // The "fast" selector couldn't handle something and bailed.
>> -            // For the purpose of debugging, just abort.
>> -            llvm_unreachable("FastISel didn't select the entire block");
>> +          Inst->dump();
>>         }
>> +        if (EnableFastISelAbort > 2)
>> +          // FastISel selector couldn't handle something and bailed.
>> +          // For the purpose of debugging, just abort.
>> +          llvm_unreachable("FastISel didn't select the entire block");
>> +
>> +        NumFastIselFailures += NumFastIselRemaining;
>>         break;
>>       }
> 
> Same here.
> 
> I hope this makes sense.
> 
> Cheers,
> Andrea