[PATCH] Remove static initializers from MCSchedModel

Tue Sep 2 10:37:24 PDT 2014

> On Sep 2, 2014, at 10:13 AM, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 
>> On Sep 2, 2014, at 10:08 AM, Andrew Trick <atrick at apple.com <mailto:atrick at apple.com>> wrote:
>> 
>>> 
>>> On Sep 2, 2014, at 9:57 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
>>> 
>>>> 
>>>> On Sep 2, 2014, at 9:49 AM, Quentin Colombet <qcolombet at apple.com <mailto: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.
>> 
>> The question is whether it is legal to use the subtarget without calling InitCPUSchedModel. This would allow a client like the disassembler to know that there is no support for CPU-level info, as opposed to providing some “phony” information.
> I think this is actually hidden from llvm-mc.  It just calls
> 
> std::unique_ptr<MCSubtargetInfo> STI(
>       TheTarget->createMCSubtargetInfo(TripleName, MCPU, FeaturesStr));
> 
> which ultimately calls InitCPUSchedModel.
> 
> The only way I can think that the CPUModel would currently be uninitialized is if the target itself didn’t call InitCPUSchedModel, and I think that would actually be a bug.
>> 
>> With codegen (llc), it’s a little different. We actually need to pick a CPU model to make MI passes happy. That’s why we have the default model.
> Good point.  The default actually makes more sense in terms of llc.
>> 
>>>>> 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.
>> 
>> I was thinking the same thing. The only thing I wasn’t sure of is whether we wanted to support invoking the disassembler without initializing the CPU.
> Yeah.  I think it makes sense for the disassembler to know that there was no model for the given CPU (or no CPU given at all).  I’m not quite sure how that interface should look.  Will have to think about it…
Looks like everything is actually ok after my patch.  The code in Disassembler.cpp now reads

if (!SCModel.hasInstrSchedModel())

which is true in the case of the default model.  That is, from the initializer I gave earlier today, the scheduling table is null for the default model.  So there’s no change of behavior to the disassembler with this patch.

Here’s a rebased patch.  Comments welcome.

Thanks,
Pete

> 
> Thanks,
> Pete
>> 
>> -Andy
>> 
>>> 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
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/c67678bc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mcschedmodel.patch
Type: application/octet-stream
Size: 25571 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/c67678bc/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140902/c67678bc/attachment-0001.html>