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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
