On 08/14/2015 11:02 AM, Richard Biener wrote:
> So the last time I did similar refactoring I wondered if we can somehow avoid
> the "noise" in non-IPA passes. Ideas I came up with are
>
> a) Inherit gimple/rtl pass classes from a class which is initialized with
> the
> function the pass operates on and provides methods like
>
> bool dom_info_available_p (..) { return dom_info_available_p (fn, ...); }
>
> thus wraps APIs working on a specific function.
>
> b) Do sth similar but make it work with overloads and clever (no idea what!)
> C++ that disables them if this_fn cannot be looked up
>
> template <disable-me-if-this_fn-cannot_be_lookedup-at-instantiation-place>
> bool dom_info_available_p (..., struct function *fn = this_fn);
>
I attached some sketch of what this "clever C++" could look like. It still
requires some changes in classes which would use this new base class, but they
are not very significant compared to converting pass functions into members of
these classes.
> all of the above would of course require that passes make all their
> implementation
> be methods of their pass class. So even more refactoring.
The good thing is that this can be done incrementally, i.e. changing one pass at
a time.
> Refactoring APIs that are used from IPA and make push/pop_cfun necessary
> for them are another thing. (there are still plenty left I think)
What I could find is:
1. ipa_analyze_node
2. some transactional memory-related stuff: ipa_tm_scan_irr_function,
ipa_tm_transform_transaction, ipa_tm_transform_clone, ipa_tm_execute.
3. tree_profiling
Presumably there are no more calculate_dominance_info/free_dominance_info call
sites with push_cfun/pop_cfun nearby (except for passes.c, but they are not
related to IPA). So now I need to figure out, what other global state (which is
set up by push_cfun) can these functions use? Could you give some advice about
that, in sense, what should I look for (e.g., push_cfun calls some target hooks,
but I did not try to look in detail, what kind of actions are performed by it)?
As for the API. I propose to, at least, remove uses of cfun from dominance.c
itself, but provide helper functions which will allow to leave the callers
unchanged, but make it possible to use all dominance-related API on functions
other than cfun.
Does such change sound reasonable to you?
On 08/14/2015 11:11 PM, David Malcolm wrote:
> The JIT guards all access to GCC's state in a big mutex ("jit_mutex", in
> gcc/jit/jit-playback.c).
Yeah, I know, I looked through JIT documentation briefly.
>
> For example, this includes everything to do with GC and GTY, since
> there's implicitly a single set of GC roots and the GC code isn't thread
> safe.
And GGC is a "stop-the-world" collector, which is just not designed for
multithreaded environment. (sigh)
As for the patch, I think reviewing this variant does not make much sense,
because of Richard suggestion on how to avoid changing the callers. But still
thanks for looking.
>
> I've been tackling things on an as-needed basis - for example, the
> recent timevar global-state removal was motivated by wanting to expose a
> profiling API to jit client code.
>
By the way, speaking of timevars. I wrote about my concerns w.r.t. them, but did
not get any feedback:
https://gcc.gnu.org/ml/gcc/2015-07/msg00165.html
Briefly speaking, I noticed that our timers introduce rather significant
measurement errors and proposed some ideas about reducing them. What do you
think?
--
Regards,
Mikhail Maltsev
#include <stdio.h>
//===- various headers - Existing GCC code --------------------------------===//
//
// No changes required
//===----------------------------------------------------------------------===//
enum cdi_direction {
CDI_DOMINATORS,
CDI_POSTDOMINATORS
};
struct basic_block_def { };
typedef basic_block_def *basic_block;
struct function
{
};
function *cfun;
class gimple_opt_pass
{
public:
virtual unsigned int execute(function *) = 0;
};
//===- dominance.h - Refactored version -----------------------------------===//
//
// Allow passing function as an additional parameter (with default value), so
// that dominaince.c does not directly use cfun any more.
//===----------------------------------------------------------------------===//
// We will need cfun to be in scope. Alternatively, we can put a 3-argument
// overload of domainted_by_p in another header, where cfun is available.
extern function *cfun;
bool
dominated_by_p(cdi_direction, basic_block, basic_block, function *fun = cfun)
{
(void)fun;
printf("\t4-argument overload\n");
return false;
}
//===- old_pass.c - This pass uses cfun -----------------------------------===//
//
// We don't need to change anything here
//===----------------------------------------------------------------------===//
class old_pass : public gimple_opt_pass
{
public:
virtual unsigned int execute(function *);
};
void old_pass_do_stuff()
{
basic_block bb1 = 0, bb2 = 0;
// ...
if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) {
// ...
}
}
unsigned int
old_pass::execute(function *fun)
{
printf("running old_pass::execute\n");
cfun = fun;
old_pass_do_stuff();
printf("finished old_pass::execute\n");
return 0;
}
//===- dom_info_mixin.h - Header file for the new API ---------------------===//
//
// dom_info_mixin template provides helper methods for manipulating
// dominance information of the function currently being compiled
//===----------------------------------------------------------------------===//
template<typename Pass>
class dom_info_mixin
{
protected:
bool dominated_by_p(cdi_direction dir, basic_block bb1, basic_block bb2) const
{
printf("\tdom_info_mixin: using m_this_fn ... -> ");
return ::dominated_by_p(dir, bb1, bb2, static_cast<const Pass *>(this)->m_this_fn);
}
};
// protection against unintended use of cfun (see new_pass_do_other_stuff)
bool dominated_by_p(cdi_direction dir, basic_block bb1, basic_block bb2);
//===- new_pass.cc - Example pass which uses the new API ------------------===//
//
// new_pass stores a pointer to current function in m_this_fn (so we get rid
// of the global shared state - at least some part of it) and uses small
// accessor methods, so we don't have to write "m_this_fn" every time.
//===----------------------------------------------------------------------===//
class new_pass : public gimple_opt_pass,
public dom_info_mixin<new_pass>
{
public:
friend class dom_info_mixin<new_pass>; // or just make m_this_fn public
virtual unsigned int execute(function *);
private:
void do_stuff();
function *m_this_fn;
};
static void new_pass_do_other_stuff();
unsigned int
new_pass::execute(function *fun)
{
printf("running new_pass::execute\n");
m_this_fn = fun;
do_stuff();
new_pass_do_other_stuff();
printf("finished new_pass::execute\n");
return 0;
}
void
new_pass::do_stuff()
{
basic_block bb1 = 0, bb2 = 0;
// ...
if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) {
// ...
}
}
static void
new_pass_do_other_stuff()
{
#if 0
basic_block bb1 = 0, bb2 = 0;
// ...
// error: call of overloaded âdominated_by_p(cdi_direction, basic_block_def*&, basic_block_def*&)â is ambiguous
if (dominated_by_p(CDI_DOMINATORS, bb1, bb2)) {
// ...
}
#endif
}
int main()
{
function fun;
old_pass old_pass_instance;
old_pass_instance.execute(&fun);
new_pass new_pass_instance;
new_pass_instance.execute(&fun);
return 0;
}
