Hello Nikos Nikoleris,
I'd like you to do a code review. Please visit
https://gem5-review.googlesource.com/2661
to review the following change.
Change subject: power: Add error checking to MathExprPowerModel
......................................................................
power: Add error checking to MathExprPowerModel
MathExprPower model currently doesn't print any useful error messages
if an expression fails to evalate. To add insult to injury, the model
only detects a failure when dumping stats and not at
initialization. This change adds a verification step in startup() that
ensures that all of the referenced stats actually exist.
Change-Id: I8f71c73341578d5882c8d93e482f5383fbda5f1d
Signed-off-by: Andreas Sandberg <[email protected]>
Reviewed-by: Nikos Nikoleris <[email protected]>
Reviewed-by: Sascha Bischoff <[email protected]>
---
M src/sim/power/mathexpr_powermodel.cc
M src/sim/power/mathexpr_powermodel.hh
2 files changed, 78 insertions(+), 18 deletions(-)
diff --git a/src/sim/power/mathexpr_powermodel.cc
b/src/sim/power/mathexpr_powermodel.cc
index 7bd2186..13d225e 100644
--- a/src/sim/power/mathexpr_powermodel.cc
+++ b/src/sim/power/mathexpr_powermodel.cc
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016 ARM Limited
+ * Copyright (c) 2016-2017 ARM Limited
* All rights reserved
*
* The license below extends only to copyright in the software and shall
@@ -46,7 +46,7 @@
#include "sim/sim_object.hh"
MathExprPowerModel::MathExprPowerModel(const Params *p)
- : PowerModelState(p), dyn_expr(p->dyn), st_expr(p->st)
+ : PowerModelState(p), dyn_expr(p->dyn), st_expr(p->st), failed(false)
{
// Calculate the name of the object we belong to
std::vector<std::string> path;
@@ -65,7 +65,49 @@
for (auto & i: Stats::statsList())
if (i->name.find(basename) == 0)
stats_map[i->name.substr(basename.size())] = i;
+
+ tryEval(st_expr);
+ const bool st_failed = failed;
+
+ tryEval(dyn_expr);
+ const bool dyn_failed = failed;
+
+ if (st_failed || dyn_failed) {
+ const auto *p = dynamic_cast<const Params *>(params());
+ assert(p);
+
+ fatal("Failed to evaluate power expressions:\n%s%s%s\n",
+ st_failed ? p->st : "",
+ st_failed && dyn_failed ? "\n" : "",
+ dyn_failed ? p->dyn : "");
+ }
}
+
+double
+MathExprPowerModel::eval(const MathExpr &expr) const
+{
+ const double value = tryEval(expr);
+
+ // This shouldn't happen unless something went wrong the equations
+ // were verified in startup().
+ panic_if(failed, "Failed to evaluate power expression '%s'\n",
+ expr.toStr());
+
+ return value;
+}
+
+double
+MathExprPowerModel::tryEval(const MathExpr &expr) const
+{
+ failed = false;
+ const double value = expr.eval(
+ std::bind(&MathExprPowerModel::getStatValue,
+ this, std::placeholders::_1)
+ );
+
+ return value;
+}
+
double
MathExprPowerModel::getStatValue(const std::string &name) const
@@ -77,12 +119,19 @@
return _temp;
// Try to cast the stat, only these are supported right now
- Info *info = stats_map.at(name);
+ const auto it = stats_map.find(name);
+ if (it == stats_map.cend()) {
+ warn("Failed to find stat '%s'\n", name);
+ failed = true;
+ return 0;
+ }
- ScalarInfo *si = dynamic_cast<ScalarInfo*>(info);
+ const Info *info = it->second;
+
+ auto si = dynamic_cast<const ScalarInfo *>(info);
if (si)
return si->value();
- FormulaInfo *fi = dynamic_cast<FormulaInfo*>(info);
+ auto fi = dynamic_cast<const FormulaInfo *>(info);
if (fi)
return fi->total();
diff --git a/src/sim/power/mathexpr_powermodel.hh
b/src/sim/power/mathexpr_powermodel.hh
index 5c121c7..b4d0254 100644
--- a/src/sim/power/mathexpr_powermodel.hh
+++ b/src/sim/power/mathexpr_powermodel.hh
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2016 ARM Limited
+ * Copyright (c) 2016-2017 ARM Limited
* All rights reserved
*
* The license below extends only to copyright in the software and shall
@@ -66,24 +66,14 @@
*
* @return Power (Watts) consumed by this object (dynamic component)
*/
- double getDynamicPower() const {
- return dyn_expr.eval(
- std::bind(&MathExprPowerModel::getStatValue,
- this, std::placeholders::_1)
- );
- }
+ double getDynamicPower() const { return eval(dyn_expr); }
/**
* Get the static power consumption.
*
* @return Power (Watts) consumed by this object (static component)
*/
- double getStaticPower() const {
- return st_expr.eval(
- std::bind(&MathExprPowerModel::getStatValue,
- this, std::placeholders::_1)
- );
- }
+ double getStaticPower() const { return eval(st_expr); }
/**
* Get the value for a variable (maps to a stat)
@@ -99,6 +89,23 @@
void regStats();
private:
+ /**
+ * Evaluate an expression in the context of this object, fatal if
+ * evaluation fails.
+ *
+ * @param expr Expression to evaluate
+ * @return Value of expression.
+ */
+ double eval(const MathExpr &expr) const;
+
+ /**
+ * Evaluate an expression in the context of this object, set
+ * failed if evaluation failed.
+ *
+ * @param expr Expression to evaluate
+ * @return Value of expression.
+ */
+ double tryEval(const MathExpr &expr) const;
// Math expressions for dynamic and static power
MathExpr dyn_expr, st_expr;
@@ -108,6 +115,10 @@
// Map that contains relevant stats for this power model
std::unordered_map<std::string, Stats::Info*> stats_map;
+
+ // Did the expression fail to evaluate (e.g., because a stat value
+ // can't be resolved)
+ mutable bool failed;
};
#endif
--
To view, visit https://gem5-review.googlesource.com/2661
To unsubscribe, visit https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8f71c73341578d5882c8d93e482f5383fbda5f1d
Gerrit-Change-Number: 2661
Gerrit-PatchSet: 1
Gerrit-Owner: Andreas Sandberg <[email protected]>
Gerrit-Reviewer: Nikos Nikoleris <[email protected]>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
