[PATCH] Remove static initializers from MCSchedModel

[Pete Cooper]

> On Sep 2, 2014, at 9:49 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
> 
> On Aug 29, 2014, at 5:20 PM, Andrew Trick <atrick at apple.com <mailto:atrick at apple.com>> wrote:
> 
>> 
>>> On Aug 29, 2014, at 4:54 PM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>>> 
>>>> I think you should remove isDefaultModel. I don’t see any purpose and it’s not maintainable.
>>> Actually, I think you’re right.  I was looking to replace the code in Disassembler.cpp which used to do ‘if (!SCModel…)’.  However, a null model in the old code didn’t even mean default, but instead uninitialized.
>>> 
>>> Would it be fair to add another field which tells us if a model is initialized?  Otherwise i’m not going to be able to handle this case?  Perhaps I can fold it in to the ‘CompleteModel’ bool and have an enum for whether a model is ‘Uninitialized, Complete, InComplete’.
>> 
>> MCSubTargetInfo may want to support uninitialized sched model to handle invoking the disassembler with missing cpu info. I’m copying Quentin to confirm.
> 
> Agreed.
I’m stepping through this in lldb and for missing/incorrect CPU strings, we still go in to MCSubtargetInfo::getSchedModelForCPU.  The current implementation of this would ultimately print an error then return the default CPU model.  It later asserts that if a CPU string is found, that the model is not null.

So it looks like null MCSchedModel’s were only currently possible if the target didn’t call InitCPUSchedModel.  That would be a bug anyway and they’d almost certainly have to be constructing the MCSubtargetInfo manually as tablegen does contain all the correct calls.
> 
>> 
>> In that case it should deal with the uninitialized state internally (it’s really the subtarget itself that is being used without proper initialization). I don’t think MCSchedModel should be impacted. The least confusing way to do this is with a null MCSchedModel pointer and asserts in the right places. If there’s some problem with that, it could just have a flag that says “don’t use the sched model”.
> 
Given the above analysis I just gave, do you think there’s anything to be done here?  it looks like a null model was never actually hit in the disassembler.

If we do want to support checking for an uninitialized model, i’m tempted to wrap it up in Optional to avoid carrying around another field for whether its initialized.  This would mean that MCSubtargetInfo::getSchedModel() would need to return the Optional<MCSchedModel> as its the only place which knows for sure whether it was initialized.  This would impact more than just the disassembler, but would be the safest thing to do.

Thanks,
Pete
> Agreed too.
> 
> Thanks,
> -Quentin
>> 
>>>> 
>>>> It would be nice to move the default values from MCSchedule into targetSchedule.td. Then we can get rid of the confusing "-1 == use default” logic and consolidate comments. Hopefully that’s possible after your changes.
>>> Yes, that change can be done if we can find a way to construct a default model in tablegen instead of the GetDefaultModel method.  Note that GetDefaultModel is really just equivalent to the old default constructor.  So we’re no closer to moving the defaults to td, but no farther either.
>> 
>> Yeah, to make that work, it looks like all clients would need to get the default sched model from target specific code. But the generic TargetSchedule and InstrItineraryData are instantiating default models.
>> 
>> -Andy
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/7a04a2e4/attachment.html>