Mordante marked 8 inline comments as done.
Mordante added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+ let Spellings = [CXX11<"", "likely", 201803>];
+ let Documentation = [LikelihoodDocs];
----------------
aaron.ballman wrote:
> Mordante wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > Hmm, I'm on the fence about specifying `201803` for these
> > > > > > attributes. Given that this is only the start of supporting the
> > > > > > attribute, do we want to claim it already matches the standard's
> > > > > > behavior? Or do we just want to return `1` to signify that we
> > > > > > understand this attribute but we don't yet fully support it in
> > > > > > common cases (such as on labels in switch statements, etc)?
> > > > > >
> > > > > > As another question, should we consider adding a C2x spelling
> > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this
> > > > > > functionality to C?
> > > > > I was also somewhat on the fence, for now I'll change it to specify
> > > > > `1`.
> > > > >
> > > > > I'll have a look at the C2x changes. Just curious do you know whether
> > > > > there's a proposal to add this to C2x?
> > > > > I'll have a look at the C2x changes. Just curious do you know whether
> > > > > there's a proposal to add this to C2x?
> > > >
> > > > There isn't one currently because there is no implementation experience
> > > > with the attributes in C. This is a way to get that implementation
> > > > experience so it's easier to propose the feature to WG14.
> > > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > >
> > > There seems to be an issue using the `1` so I used `2` instead. (Didn't
> > > want to look closely at the issue.)
> > >
> > > > I'll have a look at the C2x changes. Just curious do you know whether
> > > > there's a proposal to add this to C2x?
> > >
> > > There isn't one currently because there is no implementation experience
> > > with the attributes in C. This is a way to get that implementation
> > > experience so it's easier to propose the feature to WG14.
> >
> > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I
> > didn't implement it yet. I intend to look at it later. I first want the
> > initial part done to see whether this is the right approach.
> What issues are you running into? 1 is the default value when you don't
> specify a value specifically.
It give the error "clang/include/clang/Basic/Attr.td:265:7: error: Standard
attributes must have valid version information."
================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+ let Spellings = [CXX11<"", "likely", 201803>];
+ let Documentation = [LikelihoodDocs];
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Mordante wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Hmm, I'm on the fence about specifying `201803` for these
> > > > > > > attributes. Given that this is only the start of supporting the
> > > > > > > attribute, do we want to claim it already matches the standard's
> > > > > > > behavior? Or do we just want to return `1` to signify that we
> > > > > > > understand this attribute but we don't yet fully support it in
> > > > > > > common cases (such as on labels in switch statements, etc)?
> > > > > > >
> > > > > > > As another question, should we consider adding a C2x spelling
> > > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this
> > > > > > > functionality to C?
> > > > > > I was also somewhat on the fence, for now I'll change it to specify
> > > > > > `1`.
> > > > > >
> > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > whether there's a proposal to add this to C2x?
> > > > > > I'll have a look at the C2x changes. Just curious do you know
> > > > > > whether there's a proposal to add this to C2x?
> > > > >
> > > > > There isn't one currently because there is no implementation
> > > > > experience with the attributes in C. This is a way to get that
> > > > > implementation experience so it's easier to propose the feature to
> > > > > WG14.
> > > > > I was also somewhat on the fence, for now I'll change it to specify
> > > > > `1`.
> > > >
> > > > There seems to be an issue using the `1` so I used `2` instead. (Didn't
> > > > want to look closely at the issue.)
> > > >
> > > > > I'll have a look at the C2x changes. Just curious do you know whether
> > > > > there's a proposal to add this to C2x?
> > > >
> > > > There isn't one currently because there is no implementation experience
> > > > with the attributes in C. This is a way to get that implementation
> > > > experience so it's easier to propose the feature to WG14.
> > >
> > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I
> > > didn't implement it yet. I intend to look at it later. I first want the
> > > initial part done to see whether this is the right approach.
> > What issues are you running into? 1 is the default value when you don't
> > specify a value specifically.
> It give the error "clang/include/clang/Basic/Attr.td:265:7: error: Standard
> attributes must have valid version information."
> > > I'll have a look at the C2x changes. Just curious do you know whether
> > > there's a proposal to add this to C2x?
> >
> > There isn't one currently because there is no implementation experience
> > with the attributes in C. This is a way to get that implementation
> > experience so it's easier to propose the feature to WG14.
>
> Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I
> didn't implement it yet. I intend to look at it later. I first want the
> initial part done to see whether this is the right approach.
I had another look and I got it working in C.
================
Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif
----------------
aaron.ballman wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Mordante wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > I'd like to see a case like `if (x) [[likely]] i=1;` just to
> > > > > > > prove that it works on statements that aren't blocks or empty
> > > > > > > statements. (My understanding is that this should already work
> > > > > > > with your current patch.)
> > > > > > >
> > > > > > > I'd like to see a case like `if (x) { [[likely]] i=1; }` to prove
> > > > > > > that it works on arbitrary statements. This //should// have the
> > > > > > > same effect as `if (x) [[likely]] { i=1; }`. My understanding is
> > > > > > > that your current patch doesn't get us there //yet//. If it's
> > > > > > > unclear how we'd get there by proceeding along your current
> > > > > > > trajectory, then I would question whether we want to commit to
> > > > > > > this trajectory at all, yet.
> > > > > > I agree it would be a good idea to add a test like `if (x)
> > > > > > [[likely]] i=1;` but I don't feel adding an additional test in this
> > > > > > file proves anything. I'll add a test to
> > > > > > `clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp` to
> > > > > > prove it not only accepts the code but also honours the attribute.
> > > > > > This is especially important due to your correct observation that
> > > > > > `if (x) { [[likely]] i=1; }` doesn't have any effect. The code is
> > > > > > accepted but the CodeGen won't honour the attribute.
> > > > > >
> > > > > > I think we can use the current approach, but I need to adjust
> > > > > > `getLikelihood`. Instead of only testing whether the current `Stmt`
> > > > > > is an `AttributedStmt` it needs to iterate over all `Stmt`s and
> > > > > > test them for the attribute. Obviously it should avoid looking into
> > > > > > `Stmt`s that also use the attribute, e.g:
> > > > > > ```
> > > > > > if(b) {
> > > > > > switch(c) {
> > > > > > [[unlikely]] case 0: ... break; // The attribute "belongs"
> > > > > > to the switch not to the if
> > > > > > [[likely]] case 1: ... break; // The attribute "belongs" to
> > > > > > the switch not to the if
> > > > > > }
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > This is especially important due to your correct observation that
> > > > > > if (x) { [[likely]] i=1; } doesn't have any effect.
> > > > >
> > > > > This code should diagnose the attribute as being ignored.
> > > > What I understood from Arthur and rereading the standard this should
> > > > work, but the initial version of the patch didn't handle this properly.
> > > > What I understood from Arthur and rereading the standard this should
> > > > work, but the initial version of the patch didn't handle this properly.
> > >
> > > My original belief was that it's acceptable for the attribute to be
> > > placed there in terms of syntax, but the recommended practice doesn't
> > > apply to this case because there's no alternative paths of execution once
> > > you've entered the compound statement. That means:
> > > ```
> > > if (x) [[likely]] { i = 1; }
> > > // is not the same as
> > > if (x) { [[likely]] i = 1; }
> > > ```
> > > That said, I can squint at the words to get your interpretation and your
> > > interpretation matches what's in the original paper
> > > (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html#examples).
> > >
> > > Ultimately, I think we should strive for consistency between
> > > implementations, and I think we should file an issue with WG21 to clarify
> > > the wording once we figure out how implementations want to interpret
> > > questions like these.
> > I don't know WG21's actual rationale, but I can imagine a programmer
> > wanting to annotate something like this:
> >
> > #define FATAL(msg) [[unlikely]] log_and_abort("Fatal error!", msg);
> > ...
> > auto packet = receive();
> > if (packet.malformed()) {
> > save_to_log(packet);
> > FATAL("malformed packet");
> > }
> > ...
> >
> > The "absolute unlikeliness" of the malformed-packet codepath is
> > encapsulated within the `FATAL` macro so that the programmer doesn't have
> > to tediously repeat `[[unlikely]]` at some branch arbitrarily far above the
> > `FATAL` point. Compilers actually do this already, every time they see a
> > `throw` — every `throw` is implicitly "unlikely" and taints its whole
> > codepath. We want to do something similar for `[[unlikely]]`.
> >
> > It's definitely going to be unsatisfyingly fuzzy logic, though, similar to
> > how inlining heuristics and `inline` are today.
> >
> > My understanding is that
> >
> > if (x) { [[likely]] i = 1; }
> > if (x) [[likely]] { i = 1; }
> >
> > have exactly the same surface reading: the same set of codepaths exist in
> > both cases, and the same "x true, i gets 1" codepath is `[[likely]]` in
> > both cases. Of course your heuristic might //choose// to treat them
> > differently, just as you might //choose// to treat
> >
> > struct S { int f() { ... } };
> > struct S { inline int f() { ... } };
> >
> > differently for inlining purposes despite their identical surface readings.
> > Compilers actually do this already, every time they see a throw — every
> > throw is implicitly "unlikely" and taints its whole codepath. We want to do
> > something similar for [[unlikely]].
>
> Heh, fun story, one of the use cases I was told was critical when discussing
> this feature in the committee came from the safety-critical space where they
> want to mark `catch` blocks as being likely in order to force the optimizer
> to do its best on the failure path, because that's the code path which had
> the hard requirements for bailing out quickly (so the elevator doesn't kill
> you, car doesn't crash, etc). They needed to be able to write:
> ```
> try {
> ...
> } catch (something& s) [[likely]] {
> ...
> } catch (...) [[likely]] {
> ...
> }
> ```
>
> > My understanding is that ... have exactly the same surface reading
>
> I can read the standard to get to that understanding, but am skeptical that
> programmers will be able to predict what these attributes do anywhere they're
> written for anything but contrived examples with that interpretation. I think
> limiting the impact to only being at branch points is at least explicable
> behavior, otherwise programmers are left reasoning through code like:
> ```
> auto what = []{ FATAL("malformed packet"); };
> if (foo) {
> what();
> } else {
> if (something) {
> what();
> }
> }
> ```
> (Does this make the `foo` branch unlikely? What impact is there on the `else`
> branch?) Now replace the lambda with a call to an inline function, or a GNU
> statement expression, an RAII constructor you can see the definition of, etc
> and ask the same questions.
>
> My fear is that by going with the least restrictive design, programmers will
> eventually treat these attributes the same way they treat use of the
> `register` keyword -- something to be avoided because the best you can hope
> for is the implementation ignores it.
>
> This isn't to say that I'm *opposed* to that approach, it's more that I'm
> skeptical of it unless we have some implementation agreement between major
> compilers about how to set user expectations appropriately.
> > Compilers actually do this already, every time they see a throw — every
> > throw is implicitly "unlikely" and taints its whole codepath. We want to do
> > something similar for [[unlikely]].
>
> Heh, fun story, one of the use cases I was told was critical when discussing
> this feature in the committee came from the safety-critical space where they
> want to mark `catch` blocks as being likely in order to force the optimizer
> to do its best on the failure path, because that's the code path which had
> the hard requirements for bailing out quickly (so the elevator doesn't kill
> you, car doesn't crash, etc). They needed to be able to write:
> ```
> try {
> ...
> } catch (something& s) [[likely]] {
> ...
> } catch (...) [[likely]] {
> ...
> }
> ```
Interesting use case, not sure how feasible it would be to implement it. I
don't know how much it would affect the implementation if the exception
handling. I tested with GCC and ICC and this doesn't compile. Looking at the
standard it shouldn't compile. `catch` http://eel.is/c++draft/except.pre
requires a compound statement http://eel.is/c++draft/stmt.block. (Obviously the
attribute can be placed in the compound statement.)
A use-case I found where placing the attribute inside the compound statement is
when using `goto`. I think this might be especially interesting when using it
in C code.
```
int foo()
{
int result = 0;
struct bar *ptr;
ptr = malloc(sizeof(struct bar));
if(!ptr) {
result = -ENOMEM;
goto err;
}
...
if(!f(ptr)) {
result = -ENOMDEV;
goto err_free;
}
[[unlikely]] err_free:
free(ptr);
[[unlikely]] err:
return result;
}
```
This allows to mark the error paths as `[[unlikely]]` by only placing the
attribute on the label and every `goto` will mark the branch unlikely.
================
Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Quuxplusone wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > Mordante wrote:
> > > > > > > Quuxplusone wrote:
> > > > > > > > I'd like to see a case like `if (x) [[likely]] i=1;` just to
> > > > > > > > prove that it works on statements that aren't blocks or empty
> > > > > > > > statements. (My understanding is that this should already work
> > > > > > > > with your current patch.)
> > > > > > > >
> > > > > > > > I'd like to see a case like `if (x) { [[likely]] i=1; }` to
> > > > > > > > prove that it works on arbitrary statements. This //should//
> > > > > > > > have the same effect as `if (x) [[likely]] { i=1; }`. My
> > > > > > > > understanding is that your current patch doesn't get us there
> > > > > > > > //yet//. If it's unclear how we'd get there by proceeding along
> > > > > > > > your current trajectory, then I would question whether we want
> > > > > > > > to commit to this trajectory at all, yet.
> > > > > > > I agree it would be a good idea to add a test like `if (x)
> > > > > > > [[likely]] i=1;` but I don't feel adding an additional test in
> > > > > > > this file proves anything. I'll add a test to
> > > > > > > `clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp` to
> > > > > > > prove it not only accepts the code but also honours the
> > > > > > > attribute. This is especially important due to your correct
> > > > > > > observation that `if (x) { [[likely]] i=1; }` doesn't have any
> > > > > > > effect. The code is accepted but the CodeGen won't honour the
> > > > > > > attribute.
> > > > > > >
> > > > > > > I think we can use the current approach, but I need to adjust
> > > > > > > `getLikelihood`. Instead of only testing whether the current
> > > > > > > `Stmt` is an `AttributedStmt` it needs to iterate over all
> > > > > > > `Stmt`s and test them for the attribute. Obviously it should
> > > > > > > avoid looking into `Stmt`s that also use the attribute, e.g:
> > > > > > > ```
> > > > > > > if(b) {
> > > > > > > switch(c) {
> > > > > > > [[unlikely]] case 0: ... break; // The attribute
> > > > > > > "belongs" to the switch not to the if
> > > > > > > [[likely]] case 1: ... break; // The attribute "belongs"
> > > > > > > to the switch not to the if
> > > > > > > }
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > This is especially important due to your correct observation that
> > > > > > > if (x) { [[likely]] i=1; } doesn't have any effect.
> > > > > >
> > > > > > This code should diagnose the attribute as being ignored.
> > > > > What I understood from Arthur and rereading the standard this should
> > > > > work, but the initial version of the patch didn't handle this
> > > > > properly.
> > > > > What I understood from Arthur and rereading the standard this should
> > > > > work, but the initial version of the patch didn't handle this
> > > > > properly.
> > > >
> > > > My original belief was that it's acceptable for the attribute to be
> > > > placed there in terms of syntax, but the recommended practice doesn't
> > > > apply to this case because there's no alternative paths of execution
> > > > once you've entered the compound statement. That means:
> > > > ```
> > > > if (x) [[likely]] { i = 1; }
> > > > // is not the same as
> > > > if (x) { [[likely]] i = 1; }
> > > > ```
> > > > That said, I can squint at the words to get your interpretation and
> > > > your interpretation matches what's in the original paper
> > > > (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html#examples).
> > > >
> > > > Ultimately, I think we should strive for consistency between
> > > > implementations, and I think we should file an issue with WG21 to
> > > > clarify the wording once we figure out how implementations want to
> > > > interpret questions like these.
> > > I don't know WG21's actual rationale, but I can imagine a programmer
> > > wanting to annotate something like this:
> > >
> > > #define FATAL(msg) [[unlikely]] log_and_abort("Fatal error!", msg);
> > > ...
> > > auto packet = receive();
> > > if (packet.malformed()) {
> > > save_to_log(packet);
> > > FATAL("malformed packet");
> > > }
> > > ...
> > >
> > > The "absolute unlikeliness" of the malformed-packet codepath is
> > > encapsulated within the `FATAL` macro so that the programmer doesn't have
> > > to tediously repeat `[[unlikely]]` at some branch arbitrarily far above
> > > the `FATAL` point. Compilers actually do this already, every time they
> > > see a `throw` — every `throw` is implicitly "unlikely" and taints its
> > > whole codepath. We want to do something similar for `[[unlikely]]`.
> > >
> > > It's definitely going to be unsatisfyingly fuzzy logic, though, similar
> > > to how inlining heuristics and `inline` are today.
> > >
> > > My understanding is that
> > >
> > > if (x) { [[likely]] i = 1; }
> > > if (x) [[likely]] { i = 1; }
> > >
> > > have exactly the same surface reading: the same set of codepaths exist in
> > > both cases, and the same "x true, i gets 1" codepath is `[[likely]]` in
> > > both cases. Of course your heuristic might //choose// to treat them
> > > differently, just as you might //choose// to treat
> > >
> > > struct S { int f() { ... } };
> > > struct S { inline int f() { ... } };
> > >
> > > differently for inlining purposes despite their identical surface
> > > readings.
> > > Compilers actually do this already, every time they see a throw — every
> > > throw is implicitly "unlikely" and taints its whole codepath. We want to
> > > do something similar for [[unlikely]].
> >
> > Heh, fun story, one of the use cases I was told was critical when
> > discussing this feature in the committee came from the safety-critical
> > space where they want to mark `catch` blocks as being likely in order to
> > force the optimizer to do its best on the failure path, because that's the
> > code path which had the hard requirements for bailing out quickly (so the
> > elevator doesn't kill you, car doesn't crash, etc). They needed to be able
> > to write:
> > ```
> > try {
> > ...
> > } catch (something& s) [[likely]] {
> > ...
> > } catch (...) [[likely]] {
> > ...
> > }
> > ```
> >
> > > My understanding is that ... have exactly the same surface reading
> >
> > I can read the standard to get to that understanding, but am skeptical that
> > programmers will be able to predict what these attributes do anywhere
> > they're written for anything but contrived examples with that
> > interpretation. I think limiting the impact to only being at branch points
> > is at least explicable behavior, otherwise programmers are left reasoning
> > through code like:
> > ```
> > auto what = []{ FATAL("malformed packet"); };
> > if (foo) {
> > what();
> > } else {
> > if (something) {
> > what();
> > }
> > }
> > ```
> > (Does this make the `foo` branch unlikely? What impact is there on the
> > `else` branch?) Now replace the lambda with a call to an inline function,
> > or a GNU statement expression, an RAII constructor you can see the
> > definition of, etc and ask the same questions.
> >
> > My fear is that by going with the least restrictive design, programmers
> > will eventually treat these attributes the same way they treat use of the
> > `register` keyword -- something to be avoided because the best you can hope
> > for is the implementation ignores it.
> >
> > This isn't to say that I'm *opposed* to that approach, it's more that I'm
> > skeptical of it unless we have some implementation agreement between major
> > compilers about how to set user expectations appropriately.
> > > Compilers actually do this already, every time they see a throw — every
> > > throw is implicitly "unlikely" and taints its whole codepath. We want to
> > > do something similar for [[unlikely]].
> >
> > Heh, fun story, one of the use cases I was told was critical when
> > discussing this feature in the committee came from the safety-critical
> > space where they want to mark `catch` blocks as being likely in order to
> > force the optimizer to do its best on the failure path, because that's the
> > code path which had the hard requirements for bailing out quickly (so the
> > elevator doesn't kill you, car doesn't crash, etc). They needed to be able
> > to write:
> > ```
> > try {
> > ...
> > } catch (something& s) [[likely]] {
> > ...
> > } catch (...) [[likely]] {
> > ...
> > }
> > ```
> Interesting use case, not sure how feasible it would be to implement it. I
> don't know how much it would affect the implementation if the exception
> handling. I tested with GCC and ICC and this doesn't compile. Looking at the
> standard it shouldn't compile. `catch` http://eel.is/c++draft/except.pre
> requires a compound statement http://eel.is/c++draft/stmt.block. (Obviously
> the attribute can be placed in the compound statement.)
>
> A use-case I found where placing the attribute inside the compound statement
> is when using `goto`. I think this might be especially interesting when using
> it in C code.
> ```
> int foo()
> {
> int result = 0;
> struct bar *ptr;
>
> ptr = malloc(sizeof(struct bar));
> if(!ptr) {
> result = -ENOMEM;
> goto err;
> }
>
> ...
> if(!f(ptr)) {
> result = -ENOMDEV;
> goto err_free;
> }
>
> [[unlikely]] err_free:
> free(ptr);
> [[unlikely]] err:
> return result;
> }
> ```
> This allows to mark the error paths as `[[unlikely]]` by only placing the
> attribute on the label and every `goto` will mark the branch unlikely.
> Ultimately, I think we should strive for consistency between implementations,
> and I think we should file an issue with WG21 to clarify the wording once we
> figure out how implementations want to interpret questions like these.
I agree having consistency would be a good. I did some testing.
It seems GCC's implementation is quite mature and does indeed handle the
attributes inside the compound statements.
It seems MSVC's implementation seems a WIP.
It seems ICC accepts the attribute but it doesn't seem to change its branch
weights.
What would be the best way to discuss this question with the other vendors? I
assume I first need to write a paper, but would it then be best to discuss this
with the other vendors or directly send it to the committee?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85091/new/
https://reviews.llvm.org/D85091
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
