Mordante planned changes to this revision.
Mordante marked 14 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:
> 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?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``. It's not recommended to annotate both branches of an ``if``
+statement with an attribute.
----------------
Quuxplusone wrote:
> aaron.ballman wrote:
> > Why? I would expect this to be reasonable code:
> > ```
> > if (foo) [[likely]] {
> > ...
> > } else if (bar) [[unlikely]] {
> > ...
> > } else if (baz) [[likely]] {
> > ...
> > } else {
> > ...
> > }
> > ```
> > Especially because I would expect this to be reasonable code:
> > ```
> > switch (value) {
> > [[likely]] case 0: ... break;
> > [[unlikely]] case 1: ... break;
> > [[likely]] case 2: ... break;
> > [[unlikely]] default: ... break;
> > }
> > ```
> > As motivating examples, consider a code generator that knows whether a
> > particular branch is likely or not and it writes out the attribute on all
> > branches. Or, something like this:
> > ```
> > float rnd_value = get_super_random_number_between_zero_and_one();
> > if (rnd_value < .1) [[unlikely]] {
> > } else if (rnd_value > .9) [[unlikely]] {
> > } else [[likely]] {
> > }
> > ```
> Right, annotating both/multiple branches of a control statement should be
> perfectly fine. Even `if (x) [[likely]] { } else [[likely]] { }` should be
> perfectly okay as far as the code generator is concerned (and we should have
> a test for that); it's silly, but there's no reason to warn against it in the
> compiler docs.
>
> Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` is not
> actually annotating "both" branches of any single `if`-statement. There
> you're annotating the true branch of an if (without annotating the else
> branch), and then annotating the true branch of another if (which doesn't
> have an else branch).
>
> Mordante, in these docs, please document the "interesting" behavior of the
> standard attribute on labels — annotating a label is different from
> annotating the labeled statement itself. In particular,
>
> [[likely]] case 1: case 2: foo(); break;
> case 3: [[likely]] case 4: foo(); break;
> case 5: case 6: [[likely]] foo(); break;
>
> indicates that the likely cases are 1, 4, 5, and 6. (1 and 4 because their
> labels are annotated; 5 and 6 because their control flow passes through an
> annotated statement. Case 3's control flow passes through an annotated label,
> but that doesn't matter to the standard attribute.)
Aaron, the current `CodeGen` code for the `switch` statement allows all
branches to have a weight, for the `if` there are only two weights allowed. As
Arthur explained the chained `if`s are different statements, so this will work
properly.
Arthur, I agree we should add the documentation about the `switch`, but I'd
rather do that when the attributes are implemented for the `switch`. For now I
think it would make sense to add some documentation about the fact that the
attribute can be placed inside the `CompoundStmt`.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1702
+
+At the moment the attribute is only implemented for an ``if`` statement.
+
----------------
aaron.ballman wrote:
> We should document whether we silently accept the attribute in other
> locations and do nothing with it (I'm not keen on this approach because it
> surprises users) or that we loudly diagnose the attribute in situations that
> we don't support it in (I think this is a more user-friendly approach, but
> may require a separate warning flag).
I'll update the documentation. I intend to continue to work on implementing
these attributes so I feel adding a temporary diagnostic is not worth the
effort.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1708
+
+ // compile with -Wimplicit-fallthrough
+ if (b) [[likely]] {
----------------
aaron.ballman wrote:
> Why does `-Wimplicit-fallthrough` matter?
It doesn't, thanks for spotting the copy-paste error.
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:659
def IgnoredAttributes : DiagGroup<"ignored-attributes">;
+def LikelihoodAttributeIf : DiagGroup<"likelihood-attribute-if">;
def Attributes : DiagGroup<"attributes", [UnknownAttributes,
----------------
aaron.ballman wrote:
> Is this because you expect people will want to control diagnostics on `if`
> statements separate from diagnostics in other syntactic locations?
This is about warning about `[[likely]]` in both branches. Due to your an
Arthur's remarks I've decided to remove them. Especially since Arthur correctly
pointed out the attribute doesn't need to be directly after the `if(x)`.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2399
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
+def err_attribute_compatibility : Error<
+ "incompatible attributes '%0' and '%1'">;
----------------
aaron.ballman wrote:
> This isn't needed, we already have `err_attributes_are_not_compatible`.
Thanks I didn't find them before, removed.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2401
+ "incompatible attributes '%0' and '%1'">;
+def note_attribute_compatibility_here : Note<
+ "attribute '%0' declarered here">;
----------------
aaron.ballman wrote:
> We've already got `note_conflicting_attribute` for this as well.
Thanks I didn't find them before, removed.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:679-680
+
+ // The code doesn't protect against the same attribute on both branches.
+ // The frontend already issued a diagnostic.
+ std::pair<bool, bool> LH = getLikelihood(Then);
----------------
aaron.ballman wrote:
> I don't think the frontend should issue a diagnostic for that situation. I
> think codegen should handle chains appropriately instead as those seem
> plausible in reasonable code.
This is the diagnostic about `if(b) [[likely]] {...} else [[likely]] {...}`.
I'll remove the diagnostic, however the CodeGen will still prioritize the
`[[likely]]` attribute in the `ThenStmt`. With Arthur's information the user
can even do more unwise things:
```
if(b) [[likely]] {
[[unlikely]] ...
} else [[likely]] {
[[unlikely]]...
}
```
If the user provided conflicting attributes the CodeGen will not fail, but
might not do what the user wanted.
This doesn't affect the chained `if`:
```
if(b) [[likely]] {
...
} else if(c) [[likely]] {
...
}
```
For the `switch` it will be allowed to annotate multiple `case`s with the same
attribute and that should work as intended. (In a `case` it won't 'protect'
against conflicting attributes in one `case`).
================
Comment at: clang/lib/Sema/SemaStmt.cpp:585
+ // Warn when both the true and false branch of an if statement have the same
+ // likelihood attribute. It's not prohibited, but makes no sense.
+ const LikelyAttr *Likely = nullptr;
----------------
Quuxplusone wrote:
> I agree with Aaron, this doesn't deserve a warning. Or at least there should
> be some thought put into the "threat model." (Are we protecting against
> someone typoing `[[likely]]`/`[[likely]]` when they meant
> `[[likely]]`/`[[unlikely]]`? But they weren't supposed to annotate both
> branches in the first place...)
Yes it protects against `[[likely]]/[[likely]]`. I've decided to remove the
warning.
================
Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif
----------------
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
}
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85091/new/
https://reviews.llvm.org/D85091
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
