[PATCH] Remove static initializers from MCSchedModel
Chandler Carruth
chandlerc at google.com
Fri Aug 29 14:40:38 PDT 2014
Regarding pointers going away, that looks really good to me, but Andy
should make that call -- i've never touched most of this code.
One minor note there:
/// getSchedModelForCPU - Get the machine model of a CPU.
///
- const MCSchedModel *getSchedModelForCPU(StringRef CPU) const;
+ MCSchedModel getSchedModelForCPU(StringRef CPU) const;
/// getSchedModel - Get the machine model for this subtarget's CPU.
///
- const MCSchedModel *getSchedModel() const { return CPUSchedModel; }
+ MCSchedModel getSchedModel() const { return CPUSchedModel; }
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.
On some of the details:
@@ -191,7 +188,6 @@ public:
bool CompleteModel;
-private:
Still not sure why its better to make all of this public...
@@ -201,37 +197,6 @@ private:
friend class InstrItineraryData;
const InstrItinerary *InstrItineraries;
-public:
- // Default's must be specified as static const literals so that
tablegenerated
- // target code can use it in static initializers. The defaults need to be
- // initialized in this default ctor because some clients directly
instantiate
- // MCSchedModel instead of using a generated itinerary.
- MCSchedModel(): IssueWidth(DefaultIssueWidth),
- MicroOpBufferSize(DefaultMicroOpBufferSize),
- LoopMicroOpBufferSize(DefaultLoopMicroOpBufferSize),
- LoadLatency(DefaultLoadLatency),
- HighLatency(DefaultHighLatency),
- MispredictPenalty(DefaultMispredictPenalty),
- PostRAScheduler(false), CompleteModel(true),
- ProcID(0), ProcResourceTable(nullptr),
- SchedClassTable(nullptr), NumProcResourceKinds(0),
- NumSchedClasses(0), InstrItineraries(nullptr) {
- (void)NumProcResourceKinds;
- (void)NumSchedClasses;
- }
Can you keep the default constructor and such now that you're not creating
globals at all?
-
- // Table-gen driven ctor.
- MCSchedModel(unsigned iw, int mbs, int lmbs, unsigned ll, unsigned hl,
- unsigned mp, bool postRASched, bool cm, unsigned pi,
- const MCProcResourceDesc *pr, const MCSchedClassDesc *sc,
- unsigned npr, unsigned nsc, const InstrItinerary *ii):
- IssueWidth(iw), MicroOpBufferSize(mbs), LoopMicroOpBufferSize(lmbs),
- LoadLatency(ll), HighLatency(hl),
- MispredictPenalty(mp), PostRAScheduler(postRASched),
- CompleteModel(cm), ProcID(pi),
- ProcResourceTable(pr), SchedClassTable(sc), NumProcResourceKinds(npr),
- NumSchedClasses(nsc), InstrItineraries(ii) {}
-
unsigned getProcessorID() const { return ProcID; }
/// Does this machine model include instruction-level scheduling.
@@ -258,6 +223,44 @@ public:
assert(SchedClassIdx < NumSchedClasses && "bad scheduling class idx");
return &SchedClassTable[SchedClassIdx];
}
+
+ // /\brief Returns a default initialiszed mdoel. Used for unknown
processors.
+ static MCSchedModel GetDefaultSchedModel() {
+ return { DefaultIssueWidth,
+ DefaultMicroOpBufferSize,
+ DefaultLoopMicroOpBufferSize,
+ DefaultLoadLatency,
+ DefaultHighLatency,
+ DefaultMispredictPenalty,
+ false,
+ true,
+ 0,
+ nullptr,
+ nullptr,
+ 0,
+ 0,
+ nullptr
+ };
+ }
I still find the default constructor somewhat cleaner than this... but it's
not a huge deal either way.
+
+ /// \brief Returns true if this model is the default,
+ bool isDefaultModel() const {
+ const MCSchedModel Default = GetDefaultSchedModel();
+ return IssueWidth == Default.IssueWidth &&
+ MicroOpBufferSize == Default.MicroOpBufferSize &&
+ LoopMicroOpBufferSize == Default.LoopMicroOpBufferSize &&
+ LoadLatency == Default.LoadLatency &&
+ HighLatency == Default.HighLatency &&
+ MispredictPenalty == Default.MispredictPenalty &&
+ PostRAScheduler == Default.PostRAScheduler &&
+ CompleteModel == Default.CompleteModel &&
+ ProcID == Default.ProcID &&
+ ProcResourceTable == Default.ProcResourceTable &&
+ SchedClassTable == Default.SchedClassTable &&
+ NumProcResourceKinds == Default.NumProcResourceKinds &&
+ NumSchedClasses == Default.NumSchedClasses &&
+ InstrItineraries == Default.InstrItineraries;
+ }
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.
On Fri, Aug 29, 2014 at 2:26 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
> On Aug 29, 2014, at 2:18 PM, Andrew Trick <atrick at apple.com> wrote:
>
>
> On Aug 29, 2014, at 2:14 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
> On Aug 29, 2014, at 2:10 PM, Andrew Trick <atrick at apple.com> wrote:
>
>
> On Aug 29, 2014, at 9:38 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>
> Hi all
>
> 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.
>
> This is part of the work to remove all the static initializers from LLVM.
>
>
>
> 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.
>
> I would define an LLVM_DELETED_FUNCTION default ctor to make sure we’re
> not silently breaking some target.
>
> LGTM.
>
> Thanks Andy. I’ll add the deleted constructor.
>
> I’m preparing an updated patch now based on Chandler’s feedback so that we
> can see how we want to proceed with this.
>
>
> 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.
>
> Yeah, i’m not sure how to do that either.
>
>
> 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.
>
> Experiment done. Everything builds with the attached patch.
>
> 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.
>
> Perhaps this is what we want. I’m open to discuss whether this is a good
> idea.
>
> Pete
>
>
>
>
> -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/20140829/6c31f7ca/attachment.html>
More information about the llvm-commits
mailing list