llvm dev - Nov 2020 - RFC: Combining Annotation Metadata and Remarks

> On Nov 6, 2020, at 17:32, Johannes Doerfert <johannesdoerfert at
gmail.com> wrote:
> 
> Cool! I really like the idea. I left a comment about metadata preservation
below.
> Once this is available we will certainly employ it to understand OpenMP
programs better.
That sounds like a great use case! Having multiple different uses cases during
the bring-up would be very helpful to make sure the system is flexible enough.
> We could also think about a user facing version of this while we are at it
;)
> 
Do you mean providing a way for users to add their own annotation to say C/C++
code?

The initial patch contains a Annotation2Metadata pass, which converts Clang
annotations (` __attribute__((annotate("_name”))` ) to `!annotation`
metadata. This allows users to use something like the snippet below, to annotate
all instructions in a function by piggybacking  on Clang’s annotate attribute.

void __attribute__((annotate("__overflow_rt_check")))
custom_overflow_check(int a, int b) { … }

>> 
>> 
>> The proposal boils down to three parts.
>> 
>> The first part consists of adding a new metadata kind (!annotation ) to
allow tagging of ‘interesting’ instructions. For example, Clang could add this
metadata to auto-init stores it generates or a library for overflowing math
could instruct Clang to add the metadata to all instructions in its overflow
check functions. !annotation metadata nodes consist of a tuple containing string
nodes indicating the type of the annotation.
>> 
>> Using metadata comes with the drawback that it can be dropped by passes
that do not know how to handle it. This means we will have to adjust existing
passes to ensure the metadata is preserved for interesting instructions, if
possible. I think this is the only intersection of the proposal with existing
code in LLVM. Loosing the metadata for instructions is also not the end of the
world. The information is provided on a best-effort basis and I am not aware of
a more robust alternative to achieve the same goals.
> 
> I think we already have the idea of "trivially preserved"
annotations and we should use that everywhere anyway.
> 
> The method that "moves/merges" metadata should look at the
annotation and decide based on the operands if
> the annotation is valid for the replacement instruction or an instruction
in the vicinity. If this sounds reasonable
> we could use a second argument, e.g., below `!0 = !{!1, i64 BIT_ENCODING}`
where the bits in BIT_ENCODING define
> properties of the annotation. One could be that it is fixed to the
instruction opcode, e.g., if you replace a store
> with a memset you drop the annotation. Though, we would add these things on
a case-by-case basis I guess.
> 
That’s a good point. It would be great if we could generalize the logic to
preserve the annotation metadata. I think for some transformations the `type` of
the annotation does not matter, but for others it might. In those cases having a
general way to encode the rules would indeed be very helpful!

I think Francis already took a look at some passes that would need updating when
annotating auto-init stores. Most of the problematic transforms (like SROA,
LoopIdiom and InstCombine) are combining stores to stores of wider types or
various mem* intrinsics. As an initial general rule, it probably makes sense to
preserve any annotation metadata, if all combined instructions share the same
metadata?

I think it would make sense to start with something like that and then go from
there and iterate once there once we have concrete use cases that need more
specialized rules. What do you think?

Cheers,
Florian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201109/36eb7c01/attachment.html>

Hi Florian,


On 11/9/20 5:09 AM, Florian Hahn wrote:
>
>> On Nov 6, 2020, at 17:32, Johannes Doerfert <johannesdoerfert at
gmail.com> wrote:
>>
>> Cool! I really like the idea. I left a comment about metadata
preservation below.
>> Once this is available we will certainly employ it to understand OpenMP
programs better.
> That sounds like a great use case! Having multiple different uses cases
during the bring-up would be very helpful to make sure the system is flexible
enough.
 From what I can tell, the scheme you proposed would certainly allow the 
first ideas that came to my mind, e.g.,
simply track how many (1) runtime calls, (2) shared variables, ..., are 
generated and how many survive.

>
>> We could also think about a user facing version of this while we are at
it ;)
>>
> Do you mean providing a way for users to add their own annotation to say
C/C++ code?
>
> The initial patch contains a Annotation2Metadata pass, which converts Clang
annotations (` __attribute__((annotate("_name”))` ) to `!annotation`
metadata. This allows users to use something like the snippet below, to annotate
all instructions in a function by piggybacking  on Clang’s annotate attribute.
>
> void __attribute__((annotate("__overflow_rt_check")))
custom_overflow_check(int a, int b) { … }
This is pretty much what I had in mind :). I didn't go over the initial 
patch so I missed it.

>
>>>
>>> The proposal boils down to three parts.
>>>
>>> The first part consists of adding a new metadata kind (!annotation
) to allow tagging of ‘interesting’ instructions. For example, Clang could add
this metadata to auto-init stores it generates or a library for overflowing math
could instruct Clang to add the metadata to all instructions in its overflow
check functions. !annotation metadata nodes consist of a tuple containing string
nodes indicating the type of the annotation.
>>>
>>> Using metadata comes with the drawback that it can be dropped by
passes that do not know how to handle it. This means we will have to adjust
existing passes to ensure the metadata is preserved for interesting
instructions, if possible. I think this is the only intersection of the proposal
with existing code in LLVM. Loosing the metadata for instructions is also not
the end of the world. The information is provided on a best-effort basis and I
am not aware of a more robust alternative to achieve the same goals.
>> I think we already have the idea of "trivially preserved"
annotations and we should use that everywhere anyway.
>>
>> The method that "moves/merges" metadata should look at the
annotation and decide based on the operands if
>> the annotation is valid for the replacement instruction or an
instruction in the vicinity. If this sounds reasonable
>> we could use a second argument, e.g., below `!0 = !{!1, i64
BIT_ENCODING}` where the bits in BIT_ENCODING define
>> properties of the annotation. One could be that it is fixed to the
instruction opcode, e.g., if you replace a store
>> with a memset you drop the annotation. Though, we would add these
things on a case-by-case basis I guess.
>>
> That’s a good point. It would be great if we could generalize the logic to
preserve the annotation metadata. I think for some transformations the `type` of
the annotation does not matter, but for others it might. In those cases having a
general way to encode the rules would indeed be very helpful!
We do this with attributes as well so that the inliner (and some others) 
now how to combine attributes. Given that we have an escape hatch (just 
drop it), it really boils down to defining reasonable "categories", 
e.g., "replace-by", "moved", "merged-with".
>
> I think Francis already took a look at some passes that would need updating
when annotating auto-init stores. Most of the problematic transforms (like SROA,
LoopIdiom and InstCombine) are combining stores to stores of wider types or
various mem* intrinsics. As an initial general rule, it probably makes sense to
preserve any annotation metadata, if all combined instructions share the same
metadata?
Keeping identical annotation, sure. Otherwise, the pass should call the 
helper API and define what kind of transformation was performed to get 
the proper set of annotations back.

>
> I think it would make sense to start with something like that and then go
from there and iterate once there once we have concrete use cases that need more
specialized rules. What do you think?
I'm all for it. Also, the things I mention here don't need to be part of
the first patch (set), just something to keep in mind.

~ Johannes

> Cheers,
> Florian

> On Nov 9, 2020, at 19:27, Johannes Doerfert <johannesdoerfert at
gmail.com> wrote:
> On 11/9/20 5:09 AM, Florian Hahn wrote:
>> 
>>> On Nov 6, 2020, at 17:32, Johannes Doerfert <johannesdoerfert at
gmail.com <mailto:johannesdoerfert at gmail.com>> wrote:
>>> 
>>> Cool! I really like the idea. I left a comment about metadata
preservation below.
>>> Once this is available we will certainly employ it to understand
OpenMP programs better.
>> That sounds like a great use case! Having multiple different uses cases
during the bring-up would be very helpful to make sure the system is flexible
enough.
> 
> From what I can tell, the scheme you proposed would certainly allow the
first ideas that came to my mind, e.g.,
> simply track how many (1) runtime calls, (2) shared variables, ..., are
generated and how many survive.
> 
> 
That sounds like it this should fit in nicely (hopefully) :)
>> 
>>> We could also think about a user facing version of this while we
are at it ;)
>>> 
>> Do you mean providing a way for users to add their own annotation to
say C/C++ code?
>> 
>> The initial patch contains a Annotation2Metadata pass, which converts
Clang annotations (` __attribute__((annotate("_name”))` ) to `!annotation`
metadata. This allows users to use something like the snippet below, to annotate
all instructions in a function by piggybacking  on Clang’s annotate attribute.
>> 
>> void __attribute__((annotate("__overflow_rt_check")))
custom_overflow_check(int a, int b) { … }
> 
> This is pretty much what I had in mind :). I didn't go over the initial
patch so I missed it.
> 
It seems to work quite well in practice, with the caveat that it is not possible
to mark blocks/statements individually. But it should be a good starting point.
> 
>> 
>>>> 
>>>> The proposal boils down to three parts.
>>>> 
>>>> The first part consists of adding a new metadata kind
(!annotation ) to allow tagging of ‘interesting’ instructions. For example,
Clang could add this metadata to auto-init stores it generates or a library for
overflowing math could instruct Clang to add the metadata to all instructions in
its overflow check functions. !annotation metadata nodes consist of a tuple
containing string nodes indicating the type of the annotation.
>>>> 
>>>> Using metadata comes with the drawback that it can be dropped
by passes that do not know how to handle it. This means we will have to adjust
existing passes to ensure the metadata is preserved for interesting
instructions, if possible. I think this is the only intersection of the proposal
with existing code in LLVM. Loosing the metadata for instructions is also not
the end of the world. The information is provided on a best-effort basis and I
am not aware of a more robust alternative to achieve the same goals.
>>> I think we already have the idea of "trivially preserved"
annotations and we should use that everywhere anyway.
>>> 
>>> The method that "moves/merges" metadata should look at
the annotation and decide based on the operands if
>>> the annotation is valid for the replacement instruction or an
instruction in the vicinity. If this sounds reasonable
>>> we could use a second argument, e.g., below `!0 = !{!1, i64
BIT_ENCODING}` where the bits in BIT_ENCODING define
>>> properties of the annotation. One could be that it is fixed to the
instruction opcode, e.g., if you replace a store
>>> with a memset you drop the annotation. Though, we would add these
things on a case-by-case basis I guess.
>>> 
>> That’s a good point. It would be great if we could generalize the logic
to preserve the annotation metadata. I think for some transformations the `type`
of the annotation does not matter, but for others it might. In those cases
having a general way to encode the rules would indeed be very helpful!
> 
> We do this with attributes as well so that the inliner (and some others)
now how to combine attributes. Given that we have an escape hatch (just drop
it), it really boils down to defining reasonable "categories", e.g.,
"replace-by", "moved", "merged-with".
It sounds like having a set to express these rules for all kinds of metadata
would be useful, not just for !annotation.
>> I think Francis already took a look at some passes that would need
updating when annotating auto-init stores. Most of the problematic transforms
(like SROA, LoopIdiom and InstCombine) are combining stores to stores of wider
types or various mem* intrinsics. As an initial general rule, it probably makes
sense to preserve any annotation metadata, if all combined instructions share
the same metadata?
> 
> Keeping identical annotation, sure. Otherwise, the pass should call the
helper API and define what kind of transformation was performed to get the
proper set of annotations back.
>> I think it would make sense to start with something like that and then
go from there and iterate once there once we have concrete use cases that need
more specialized rules. What do you think?
> 
> I'm all for it. Also, the things I mention here don't need to be
part of the first patch (set), just something to keep in mind.
Sounds good. For now, I split up the original WIP patch
(https://reviews.llvm.org/D89240 <https://reviews.llvm.org/D89240>) into
two separate ones:

1. Add !annotation metadata and AnnotationRemarks generation
https://reviews.llvm.org/D91188
2. __attribute__((annotate(“”) to !annotate metadata conversion
https://reviews.llvm.org/D91195

Cheers,
Florian
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20201110/8627506a/attachment.html>