> Am 31.12.2023 um 11:20 schrieb Jørgen Kvalsvik <j...@lambda.is>:
>
> 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 think we need to at least document the UIDs have a meaningful value between
gimplification and CFG construction. It might be that nested function
lowering, GIMPLE lowering or OMP stuff happening inbetween wreck this. We
should be able to assert all stmt UIDs are zero upon CFG construction when not
doing condition coverage?
Another way to ferry information would be via extra stmts like we use
ANNOTATE_EXPR to annotate loops before CFG construction or the profile
annotations.
Richard
> 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!
