Page MenuHomePhabricator

[llvm][STLExtras] Add various type_trait utilities currently present in MLIR
ClosedPublic

Authored by rriddle on Apr 13 2020, 2:58 PM.

Details

Summary

This revision moves several type_trait utilities from MLIR into LLVM. Namely, this revision adds:
is_detected - This matches the experimental std::is_detected
is_invocable - This matches the c++17 std::is_invocable
function_traits - A utility traits class for getting the argument and result types of a callable type

Depends On D78057

Diff Detail

Event Timeline

rriddle created this revision.Apr 13 2020, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2020, 2:58 PM
lattner accepted this revision.Apr 13 2020, 4:16 PM

LGTM (assuming the test failures being spurious).

Would it be possible/reasonable to split up LLVM's STLExtras.h?

This revision is now accepted and ready to land.Apr 13 2020, 4:16 PM

LGTM (assuming the test failures being spurious).

Would it be possible/reasonable to split up LLVM's STLExtras.h?

I think it would depend on what dependencies are specifically needed/desired, which isn't exactly clear(at least to me) at the moment. A majority of STLExtras.h is the various range classes, which would presumably still be in the same file after splitting.

Yeah it was just a thought - much of STLExtras is conceptually "iterator like", so moving them to IteratorExtras.h or something could make the rest stand out. There is also a cluster that could go to AlgorithmExtras.h etc.

Not sure if the other LLVM type traits have tests - but could you check & probably add some static_assert tests to demonstrate/validate the functionality being added here?

llvm/include/llvm/ADT/STLExtras.h
109–145

Use of this device makes me a bit uncomfortable - such a thing would be incompatible with overloading, default arguments, etc.

A general idea that's gone around inside Google but I can't find a good reference for it externally, is to only depend on the "call only interface" (ie: never take the address of a function you don't own - only call it with whatever types it was intended to have and treat it as though it returns the type it's documented to return). This ensures maximum flexibility for the function vendor to refactor it - add default arguments or overload the function as part of migrating/improving/refactoring the API, etc.

Is there any chance the existing uses of the mlir-equivalent of this trait could reasonably avoid it & instead rely on the call-only contract when interacting with functions/functors?

rriddle marked an inline comment as done.Apr 13 2020, 5:55 PM
rriddle added inline comments.
llvm/include/llvm/ADT/STLExtras.h
109–145

Thanks for the comment David. In all of the usages in MLIR, we aren't necessarily concerned about overloading/default arguments/etc. The general usage is that there is an API that takes a callback(generally expected to be a lambda), and then switches depending on the type of the function argument. Removing the ability to do that would greatly uglify a large amount of code. Is there a way to do that without doing the above? I'm open to any suggestions you might have.

dblaikie added inline comments.Apr 13 2020, 6:18 PM
llvm/include/llvm/ADT/STLExtras.h
109–145

When you say "switches depending on the type of the function argument" - would it be possible to use overloads instead? (eg: func(function_ref<void(T1)>); func(function_ref<void(T2)>); etc... )

rriddle marked an inline comment as done.Apr 13 2020, 7:24 PM
rriddle added inline comments.
llvm/include/llvm/ADT/STLExtras.h
109–145

I tried that originally, but when using overloads even the most trivial examples fail due to ambiguity:

void bar(llvm::function_ref<void(int)> fn);
void bar(llvm::function_ref<void(bool)> fn);

void foo() {
  bar([](int i) {});
}

<source>:8:3: error: call to 'bar' is ambiguous

  bar([](int i) {});

  ^~~

<source>:4:6: note: candidate function

void bar(llvm::function_ref<void(int)> fn);

     ^

<source>:5:6: note: candidate function

void bar(llvm::function_ref<void(bool)> fn);

     ^

https://godbolt.org/z/JDTFtn

dblaikie added inline comments.Apr 13 2020, 8:11 PM
llvm/include/llvm/ADT/STLExtras.h
109–145

Fair point - though are you dealing with situations where there is such an ambiguity, and you still want different behavior based on those two ambiguous alternatives? That seems like quite subtle code that might be good to avoid.

rriddle marked an inline comment as done.Apr 13 2020, 8:32 PM
rriddle added inline comments.
llvm/include/llvm/ADT/STLExtras.h
109–145

int and bool are definitely contrived examples, but they were only meant to demonstrate the current state of overload detection. The most common situation we deal with is w.r.t Operations vs. *Op classes. MLIR Ops are like smart pointers that provide an interface into a generic Operation*. Depending on the situation and availability, it is much more convenient to interface with an *Op than an Operation. For example, MLIR has visitor methods that allow for walking nested operations. These are widely used, and extremely convenient:

ModuleOp module = ...;

// Walk *all* operations within the module opaquely.
module.walk([](Operation *op) { ... });

// Wall only functions, or gpu modules, within the module.
module.walk([](FuncOp func) { ... });
module.walk([](GPUModuleOp gpuModule) { ... });

// Walk all operations that provide the memory effect interface.
module.walk([](MemoryEffectOpInterface memoryInterface) { ... });
rriddle updated this revision to Diff 257446.Apr 14 2020, 12:26 PM

Add tests for type traits

dblaikie added inline comments.Apr 14 2020, 3:08 PM
llvm/include/llvm/ADT/STLExtras.h
109–145

Fair enough - I'm a little apprehensive about adding such a general tool to LLVM's core libraries & how it might get used, but I guess we'll leave that up to code review.

(one possible other way to have structured your callback API might've been to use "moduel.walk<GPUModuleOp>([](auto gpuModule) { ... });" etc... awkward in its own way, to be sure)

rriddle marked an inline comment as done.Apr 14 2020, 3:20 PM
rriddle added inline comments.
llvm/include/llvm/ADT/STLExtras.h
109–145

I 100% agree with you that this should only really be used for specific scenarios and not a "let's slap this everywhere". Some of this was guarded by the fact that we didn't have C++14 until recently.

This revision was automatically updated to reflect the committed changes.

FYI – This is generating new warnings with top-of-tree clang. That being said, the warning itself might be faulty:

/usr/local/include/llvm/ADT/STLExtras.h:115:58: warning: HTML tag 'i' requires an end tag [-Wdocumentation-html]
///   * To access the type of an argument: Traits::arg_t<i>
                                                        ~^~
1 warning generated.