<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 2, 2014, at 9:49 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1252" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><div class="">On Aug 29, 2014, at 5:20 PM, Andrew Trick <<a href="mailto:atrick@apple.com" class="">atrick@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 29, 2014, at 4:54 PM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">I think you should remove isDefaultModel. I don’t see any purpose and it’s not maintainable.</div></div></blockquote><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">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.</span></div><div class=""><br class=""></div><div class="">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’.</div></div></div></blockquote><div class=""><br class=""></div><div class="">MCSubTargetInfo may want to support uninitialized sched model to handle invoking the disassembler with missing cpu info. I’m copying Quentin to confirm.</div></div></div></blockquote><div class=""><br class=""></div>Agreed.</div></div></div></blockquote>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.</div><div><br class=""></div><div>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.<br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><div class="">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”.</div></div></div></blockquote><div class=""><br class=""></div></div></div></div></blockquote>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.</div><div><br class=""></div><div>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.</div><div><br class=""></div><div>Thanks,</div><div>Pete<br class=""><blockquote type="cite" class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">Agreed too.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-Quentin<br class=""><blockquote type="cite" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><div class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><blockquote type="cite" class=""><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">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.</div></div></blockquote>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.</div></div></div></blockquote><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">-Andy</div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""></body></html>