On 31/12/2023 10:40, Jan Hubicka wrote:
This seems good. Profile-arcs is rarely used by itself - most of time it
is implied by -fprofile-generate and -ftest-coverage and since
condition coverage is more associated to the second, I guess
-fcondition-coverage is better name.
Since -fcondition-coverage now affects gimplification (which makes me
less worried about its ability to map things back to conditionals), it
should be marked as incompatible with -fprofile-generate and
-fprofile-use. It would only work with -fcondtion-coverage was used
both with -fprofile-generate and -fprofile-use and I do not see much use
of this. On the other hand I can imagine users wanting both coverage and
-fprofile-generate run at once, so we should output sorry message that
this is not implemented
I don't quite understand this - gimplification is affected only in the sense
that slightly more information is recorded, why would it be incompatible
with -fprofile-generate?
You are right. Originally I tought you add extra temporary boolean
variables (as described by the comment in condition coverage) but that
happens only at tree-profile time.
If condition statement is removed before profiling is done, you will end
up with a stale entry in the hash table.
We already keep EH regions and profile histogram in on-side hashtables,
so I think you need to follow gimple_.*histograms calls in
gimple-iterator.cc and be sure that the hashtable is updated.
Also if function is inlined (which may be forced at -O0 using
always_inline) the conditional annotations will be lost.
I think you should simply make condition coverage to happen before
pass_early_inline is run. But that may be done incrementally.
In the v8 revision I switched strategy to not using an auxiliary hash table,
but rather temporarily in the gimple.uid field until the basic block is
created and it can be stored there. What do you think about that approach?
https://patchwork.sourceware.org/project/gcc/patch/20231212114125.1998866-...@lambda.is/
I assume the stale entries is why I got ICEs from what looked like double
frees in gc runs.
I see, I missed version 8. Sorry for that.
I am not sure about (ab)using gimple_uid - it looks like something that
may lead to surprises later. We can check with Richi for an opinion.
It seems to me that using startegy similar to histogram annotations may
end up being more maintainable (since at least it is symetric to what we
already have).
The use of the gimple_uid is probably the most problematic piece right
now. It is documented to only being meaningful within a pass, otherwise
assumed to contain garbage. I have assumed (and tried to check, although
there is some room for error here) that gimplification is the last thing
that happens before the basic blocks are created, at which point the
information is moved there. This obviously pushes the field to its
limit, but maybe that's ok - I leave that decision to you.
I can have an extra look at the histogram implementation and see if
something falls out. I am not so worried about stale for other reasons
than double-frees because the lookup is driven by the gconds found in
the CFG, which are obviously valid, and the table would not be traversed
on its own.
I wonder if we want to have in longer term more general annotation
infratstructure like we do for symbol table.
I will check the version patch day after tomorrow, since now I am about
to set off for new year celebration...
Happy new year,
Honza
Happy new year!
