<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></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 Aug 29, 2014, at 3:07 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=""><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 2:40 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" class="">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Regarding pointers going away, that looks really good to me, but Andy should make that call -- i've never touched most of this code.<div class=""><br class=""></div><div class="">One minor note there:</div><div class=""><div class=""> /// getSchedModelForCPU - Get the machine model of a CPU.</div>
<div class=""> ///</div><div class="">- const MCSchedModel *getSchedModelForCPU(StringRef CPU) const;</div><div class="">+ MCSchedModel getSchedModelForCPU(StringRef CPU) const;</div><div class=""> </div><div class=""> /// getSchedModel - Get the machine model for this subtarget's CPU.</div>
<div class=""> ///</div><div class="">- const MCSchedModel *getSchedModel() const { return CPUSchedModel; }</div><div class="">+ MCSchedModel getSchedModel() const { return CPUSchedModel; }</div></div><div class=""><br class=""></div><div class="">These seem like they might be better as const references? At least the last one. I might have missed other places where the same is true, not sure.</div></div></div></blockquote>Good point. I’ll update all the places I can.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">
<div class=""><br class=""></div><div class="">On some of the details:</div><div class=""><br class=""></div><div class=""><div class="">@@ -191,7 +188,6 @@ public:<br class=""></div><div class=""> </div><div class=""> bool CompleteModel;</div><div class=""> </div><div class="">-private:</div><div class=""><br class=""></div><div class="">Still not sure why its better to make all of this public…</div></div></div></div></blockquote>Ultimately it was because tablegen couldn’t set the values of the private fields when it used the ‘x = { … }’ syntax.<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">
<div class=""><br class=""></div><div class=""><br class=""></div><div class="">@@ -201,37 +197,6 @@ private:<br class=""></div><div class=""> friend class InstrItineraryData;</div><div class=""> const InstrItinerary *InstrItineraries;</div><div class=""> </div><div class="">-public:</div><div class="">- // Default's must be specified as static const literals so that tablegenerated</div>
<div class="">- // target code can use it in static initializers. The defaults need to be</div><div class="">- // initialized in this default ctor because some clients directly instantiate</div><div class="">- // MCSchedModel instead of using a generated itinerary.</div>
<div class="">- MCSchedModel(): IssueWidth(DefaultIssueWidth),</div><div class="">- MicroOpBufferSize(DefaultMicroOpBufferSize),</div><div class="">- LoopMicroOpBufferSize(DefaultLoopMicroOpBufferSize),</div><div class="">
- LoadLatency(DefaultLoadLatency),</div><div class="">- HighLatency(DefaultHighLatency),</div><div class="">- MispredictPenalty(DefaultMispredictPenalty),</div><div class="">- PostRAScheduler(false), CompleteModel(true),</div>
<div class="">- ProcID(0), ProcResourceTable(nullptr),</div><div class="">- SchedClassTable(nullptr), NumProcResourceKinds(0),</div><div class="">- NumSchedClasses(0), InstrItineraries(nullptr) {</div>
<div class="">- (void)NumProcResourceKinds;</div><div class="">- (void)NumSchedClasses;</div><div class="">- }</div><div class=""><br class=""></div><div class="">Can you keep the default constructor and such now that you're not creating globals at all?</div></div></div></div></blockquote>I’m actually creating loads of globals, but with ‘= { … }’. I think those still count as static if I include the default constructor. I’ll check.<br class=""></div></div></div></blockquote>Turns out the presence of the default constructor makes these initializers fail:</div><div><br class=""></div><div>../include/llvm/MC/MCSchedule.h:231:12: error: no matching constructor for initialization of 'llvm::MCSchedModel'<br class=""> return { DefaultIssueWidth,<br class=""> ^~~~~~~~~~~~~~~~~~~~<br class="">../include/llvm/MC/MCSchedule.h:227:3: note: candidate constructor not viable: requires 0 arguments, but 14 were provided<br class=""> MCSchedModel() { }<br class=""> ^<br class="">../include/llvm/MC/MCSchedule.h:136:8: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but<br class=""> 14 were provided<br class="">struct MCSchedModel {</div><div><br class=""></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=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="">
<br class=""></div><div class="">-</div><div class="">- // Table-gen driven ctor.</div><div class="">- MCSchedModel(unsigned iw, int mbs, int lmbs, unsigned ll, unsigned hl,</div><div class="">- unsigned mp, bool postRASched, bool cm, unsigned pi,</div>
<div class="">- const MCProcResourceDesc *pr, const MCSchedClassDesc *sc,</div><div class="">- unsigned npr, unsigned nsc, const InstrItinerary *ii):</div><div class="">- IssueWidth(iw), MicroOpBufferSize(mbs), LoopMicroOpBufferSize(lmbs),</div>
<div class="">- LoadLatency(ll), HighLatency(hl),</div><div class="">- MispredictPenalty(mp), PostRAScheduler(postRASched),</div><div class="">- CompleteModel(cm), ProcID(pi),</div><div class="">- ProcResourceTable(pr), SchedClassTable(sc), NumProcResourceKinds(npr),</div>
<div class="">- NumSchedClasses(nsc), InstrItineraries(ii) {}</div><div class="">-</div><div class=""> unsigned getProcessorID() const { return ProcID; }</div><div class=""> </div><div class=""> /// Does this machine model include instruction-level scheduling.</div>
<div class="">@@ -258,6 +223,44 @@ public:</div><div class=""> assert(SchedClassIdx < NumSchedClasses && "bad scheduling class idx");</div><div class=""> return &SchedClassTable[SchedClassIdx];</div><div class=""> }</div><div class="">
+</div><div class="">+ // /\brief Returns a default initialiszed mdoel. Used for unknown processors.</div><div class="">+ static MCSchedModel GetDefaultSchedModel() {</div><div class="">+ return { DefaultIssueWidth,</div><div class="">+ DefaultMicroOpBufferSize,</div>
<div class="">+ DefaultLoopMicroOpBufferSize,</div><div class="">+ DefaultLoadLatency,</div><div class="">+ DefaultHighLatency,</div><div class="">+ DefaultMispredictPenalty,</div><div class="">+ false,</div>
<div class="">+ true,</div><div class="">+ 0,</div><div class="">+ nullptr,</div><div class="">+ nullptr,</div><div class="">+ 0,</div><div class="">+ 0,</div><div class="">+ nullptr</div><div class="">+ };</div>
<div class="">+ }</div><div class=""><br class=""></div><div class="">I still find the default constructor somewhat cleaner than this... but it's not a huge deal either way.</div></div></div></div></blockquote>Yeah, its not perfect. Ultimately it would be better to avoid any non-tablegen constructing of these values, but this is the only one I can’t eliminate right now. <br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class=""><br class=""></div><div class="">+</div><div class="">+ /// \brief Returns true if this model is the default,</div>
<div class="">+ bool isDefaultModel() const {</div><div class="">+ const MCSchedModel Default = GetDefaultSchedModel();</div><div class="">+ return IssueWidth == Default.IssueWidth &&</div><div class="">+ MicroOpBufferSize == Default.MicroOpBufferSize &&</div>
<div class="">+ LoopMicroOpBufferSize == Default.LoopMicroOpBufferSize &&</div><div class="">+ LoadLatency == Default.LoadLatency &&</div><div class="">+ HighLatency == Default.HighLatency &&</div>
<div class="">+ MispredictPenalty == Default.MispredictPenalty &&</div><div class="">+ PostRAScheduler == Default.PostRAScheduler &&</div><div class="">+ CompleteModel == Default.CompleteModel &&</div>
<div class="">+ ProcID == Default.ProcID &&</div><div class="">+ ProcResourceTable == Default.ProcResourceTable &&</div><div class="">+ SchedClassTable == Default.SchedClassTable &&</div><div class="">+ NumProcResourceKinds == Default.NumProcResourceKinds &&</div>
<div class="">+ NumSchedClasses == Default.NumSchedClasses &&</div><div class="">+ InstrItineraries == Default.InstrItineraries;</div><div class="">+ }</div></div><div class=""><br class=""></div><div class="">Maybe in a separate patch, but it would seem cleaner to define a generic operator== for the class, and then just compare against the default here.</div></div></div></blockquote>Sure. I’m happy to add that to this patch.</div><div class=""><br class=""></div><div class="">Thanks for the feedback.</div><div class="">Pete<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">
<div class=""><br class=""></div><div class=""><br class=""></div></div><div class="gmail_extra"><br class=""><br class=""><div class="gmail_quote">On Fri, Aug 29, 2014 at 2:26 PM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 29, 2014, at 2:18 PM, Andrew Trick <<a href="mailto:atrick@apple.com" target="_blank" class="">atrick@apple.com</a>> wrote:</div>
<br class=""><div class=""><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<blockquote type="cite" class=""><div class=""><br class="">On Aug 29, 2014, at 2:14 PM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>> wrote:</div><br class=""><div class=""><blockquote type="cite" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<br class="">On Aug 29, 2014, at 2:10 PM, Andrew Trick <<a href="mailto:atrick@apple.com" target="_blank" class="">atrick@apple.com</a>> wrote:<br class=""><br class=""><br class=""><blockquote type="cite" class="">On Aug 29, 2014, at 9:38 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>> wrote:<br class="">
<br class="">Hi all<br class=""><br class="">Please review this patch to change MCSchedModel to avoid static initializers. This makes it behavior match other structs with similar uses, e.g., MCWriteProcResEntry and MCProcResourceDesc.<br class=""><br class="">This is part of the work to remove all the static initializers from LLVM.<br class="">
</blockquote><br class=""><br class="">Great. I can’t remember why we needed a default ctor on MCSchedModel any more. If we can get away with constant static initialization it’s much better.<br class=""><br class="">I would define an LLVM_DELETED_FUNCTION default ctor to make sure we’re not silently breaking some target.<br class="">
<br class="">LGTM.<br class=""></blockquote><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">Thanks Andy. I’ll add the deleted constructor.</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">I’m preparing an updated patch now based on Chandler’s feedback so that we can see how we want to proceed with this.</span><br class="">
</div></blockquote><br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
Unfortunately, I don’t think the “delete” keyword will work because you can’t define a default ctor if you want to initialize it with an initializer list. Maybe someone who knows C++11 rules has an idea for how to catch default ctor uses.</div>
</div></blockquote></div>Yeah, i’m not sure how to do that either.<div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
Or you could just do an experiment where you delete the default ctor without changing the initialization and make sure all targets still build. That would be good enough for me.</div></div></blockquote></div>Experiment done. Everything builds with the attached patch.</div>
<div class=""><br class=""></div><div class="">For some details as to how this differs from before. I changed it from a static field to a function to return a default initialized model. Unfortunately this requires more source changes as now we can’t have pointers to the default model. In fact cleaning this up basically removes pointers to MCSchedModel from all the compiler. </div>
<div class=""><br class=""></div><div class="">Perhaps this is what we want. I’m open to discuss whether this is a good idea.</div><span class="HOEnZb"><font color="#888888" class=""><div class=""><br class=""></div><div class="">Pete</div><div class=""><br class=""></div><div class=""></div></font></span></div>
<br class=""><div style="word-wrap:break-word" class=""><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
<br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">
-Andy</div></div></blockquote></div><br class=""></div><br class="">_______________________________________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br class="">
<br class=""></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></body></html>