[PATCH] D91979: [Clang][Attr] Introduce the `assume` function attribute

Thu Dec 10 06:15:27 PST 2020

aaron.ballman added a comment.

In D91979#2444327 <https://reviews.llvm.org/D91979#2444327>, @jdoerfert wrote:

> In D91979#2440554 <https://reviews.llvm.org/D91979#2440554>, @aaron.ballman wrote:
>
>> Thank you for your patience with the delays getting to your review! Along with the documentation question, one line of thinking I'm also not clear on is whether this should be a type-based attribute rather than a declaration-based attribute. For instance, if you want to write an assumption about the parameters to a function, those are not in scope for a declaration attribute but are for a type attribute.
>
> I would like to see that, in our last proposal of something similar we suggested to add something like `__attribute__((arg_attr(3, "nofree")))` which would translate to the `nofree` IR attribute for argument 3. I can also envision a type based assumption attribute but I feel a declaration one will be needed nevertheless. We'll also add expression assumptions as they are required by OpenMP and generally useful. The "payload" of the assume attribute will then be a union, similar to the `AlignedAttribute`.

I want to make sure we're on the same page with this.

If the assumption is a unit of information that applies to a single parameter, then the attribute should be written on the parameter itself rather than using positional information. e.g., `void func(int i [[clang::whatever]]);` is preferable to `void func(int i) [[clang::whatever(1)]];` By writing the attribute directly on the parameter, we remove confusion over things like "are parameter indexes one-based or zero-based?" and "does the implicit 'this' parameter count?".

If the assumption is unit of information that applies to the function but involves one or more parameters, then writing the attribute *might* makes sense as a type attribute. e.g., `void func(int i, int j) [[clang::whatever(i < j)]];`

It sounds to me like we may wind up with both kinds of situations, which is fine.

>> But also, it seems like this assumption information is something that would be useful to carry around as part of the function's identity. For instance, would users want to be able to make a call through a function pointer and have the same set of assumptions conveyed to the backend?
>
> Like other IR attributes, if you can go from the function pointer to a (set of) declaration(s) you can use the attributes from that declaration. I don't think this is any different than other function attributes, e.g., `readnone` in this aspect.

But what about the behavior the user sees? e.g., should the user be alerted to this being a potentially bad thing?

  void func(int *ip) [[clang::assume(ip != 0)]];
  void other_func(int *ip); // No assumptions about ip
  int *get_ip(); // May return null

  ...
  void (*fp)(int *);

  if (rand() % 10 == 0)
    fp = other_func;
  else
    fp = func; // Should we warn about this?

  fp(get_ip()); // This could do bad things if fp == func because get_ip() may return null

================
Comment at: clang/include/clang/Basic/Attr.td:3482
+  let Spellings = [Clang<"assume">];
+  let Subjects = SubjectList<[Function]>;
+  let InheritEvenIfAlreadyPresent = 1;
----------------
Should this be supported on ObjCMethod declarations as well?

================
Comment at: clang/include/clang/Basic/AttrDocs.td:3930
+analysis and optimization passes can assume the "assumption" to hold.
+This is similar to :ref:`__builtin_assume <langext-__builtin_assume>` but instead of an
+expression that can be assumed to be non-zero, the assumption is expressed as
----------------
wrap to 80-col

================
Comment at: clang/include/clang/Basic/AttrDocs.td:3940
+
+While LLVM plugins might utilize more assumption strings, the default LLVM
+optimization passes are aware of the following assumptions:
----------------

================
Comment at: clang/include/clang/Basic/AttrDocs.td:3950
+The OpenMP standard defines the meaning of OpenMP assumptions ("omp_XYZ" is
+spelled "XYZ" in the OpenMP standard).
+
----------------
Can you link to that standard here so users don't have to hunt for it?

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:742
   "the argument to %0 has side effects that will be discarded">,
-  InGroup<DiagGroup<"assume">>;
+  InGroup<Assumption>;
+def warn_assume_attribute_string_unknown : Warning<
----------------
This changes behavior for users because `-Wno-assume` now needs to be spelled `-Wno-assumption` -- I think we may want to keep the diagnostics for the assumption attribute separate from the diagnostics for builtin assume.

For instance, I think the assumption attribute warnings will all wind up leading to an ignored attribute, so perhaps the new grouping should be under the existing ignored attributes group.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:744
+def warn_assume_attribute_string_unknown : Warning<
+  "the assumption string '%0' is not known and might be misspelled">,
+  InGroup<Assumption>;
----------------
I'd say that this case should probably be worded `unknown assumption string '%0'; attribute ignored`

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:747
+def warn_assume_attribute_string_unknown_suggested : Warning<
+  "the assumption string '%0' is not known and might be misspelled; "
+  "did you mean '%1'?">,
----------------
Similarly: `unknown assumption string '%0' may be misspelled; attribute ignored, did you mean '%1'?`

================
Comment at: clang/include/clang/Sema/Sema.h:10007
+  /// Check if \p AssumptionStr is a known assumption and warn if not.
+  void CheckAssumptionAttr(SourceLocation Loc, StringRef AssumptionStr);
+
----------------
I think this should probably return something so that the caller knows whether to drop the attribute or not?

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1681
+  StringRef Str;
+  if (AL.getNumArgs() == 1 && !S.checkStringLiteralArgumentAttr(AL, 0, Str))
+    return;
----------------
No need to check the count manually, the common attribute handler should do that for you already.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1684
+
+  S.CheckAssumptionAttr(AL.getLoc(), Str);
+
----------------
Does it make sense to warn about an unknown assumption string but then keep the attribute anyway? This seems like a situation where I'd expect the attribute to be dropped as it appears to be meaningless.

Also, I think this should pass in the source location to the argument, not the attribute itself. You can get this information out of the call to `checkStringLiteralArgumentAttr()` (it's an optional parameter).

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1689
+
+llvm::StringSet<> Sema::KnownAssumptionStrings({
+    "omp_no_openmp",          // OpenMP 5.1
----------------
The current approach is reasonable, but I wonder if there's a way we can force this list to be filled out by the backend pass authors such that the frontend can query the backend for the supported list?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91979/new/

https://reviews.llvm.org/D91979