On 13/07/15 15:43, Michael Matz wrote:
Hi,
On Sun, 12 Jul 2015, Tom de Vries wrote:
I'm trying to get to a defined policy for what is allowed for caches.
Either forbidding or allowing multi-step dependencies, I don't really
mind.
I think forbidding is the way to go, because ...
I managed to write a patch series that implements the forbidding of
multi-step dependencies. I'll post this soon.
https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00970.html
... aha, it finds bugs! So you actually had to changes some hashes to be
non-caching for this to work, and it's all some decl-to-debug-something
maps (well, of course, otherwise you wouldn't have run into the bug you're
trying to fix in the first place). I think this hints at actual bugs that
needs fixing in the gomp branch:
As you analyzed in PR 66714, eventually a decl A is replaced by decl B,
but its debug-expr is simply copied, and that one itself refers to decls
(let's call them D*) that meanwhile are removed.
Now, as the D* decls are not in any other data structure (otherwise they
would have been marked) the typical actions that needed to have been done
for them (like e.g. associating debug info with them, allocating them to
some stack or register place) i.e. anything that needed to be done for
normal decls won't have been done. So the debug info generator in this
case, when it sees those D* decls can't do its work, e.g. debug info
generated for D* won't refer to the real place containing the value,
because also the generated code itself doesn't refer to D* anymore.
This also hints at other problems (which might not actually occur in the
case at hand, but still): the contents of DECL_VALUE_EXPR is the "real"
thing containing the value of a decl (i.e. a decl having a value-expr
doesn't itself occur in the code anymore), be it a decl itself, or some
expression (which might also refer to decls). Now, in PR 66714 you
analyzed that one of those D* was removed from the function, which should
have happened only because no code referred to anymore, i.e. D* was also
rewritten to some other D'* (if it weren't rewritten and D* was referred
to in code, you would have created a miscompilation). At that point also
the DECL_VALUE_EXPRs need to be rewritten to refer to D'*, not to D*
anymore.
Thanks for looking into the PR. I suspected that these things were
wrong, but I have no knowledge of this part of the compiler, so I was
not sure.
Implementing multi-step maps or making the hashmaps non-caching doesn't
solve any of the above problems
I'm not saying that making those hashmaps non-caching solves any of
these problems.
I'm saying that it decouples fixing the policy (for which I have a
patch) from fixing the issues that allow us to use these 3 as caches
again (for which there are no patches yet). The advantage of having a
policy in place is that we won't regress for tables still marked as
cache (or new tables marked as cache). So blocking committing the policy
on those issues makes no sense IMHO.
Thanks,
- Tom
, it merely forces some DECLs in the
compiler to remain live but that actually have no meaning in their
context.
So, I think this makes it pretty clear that those hashmaps should remain
caching maps, and that multi-step deps in caches should be disallowed, and
that the underlying problem should rather be fixed (and the checking code
against multi-step-deps should be added to the compiler).
Ciao,
Michael.
