[IRGen] Improvements to function type metadata #12500

xedin · 2017-10-18T22:21:17Z

Currently only single 'inout' flag has been encoded into function
metadata, these changes extend function metadata to support up to
32 flags per parameter.

xedin · 2017-10-18T22:23:10Z

@rjmccall Do changes so far make sense to you? It seems like the next step would be to modify MetadataCache to support this new flags to unique function keys. And at the end do couple of optimizations - don't encode flags for parameters which don't have any etc.

DougGregor · 2017-10-20T17:07:49Z

include/swift/AST/Types.h

+    return ParameterTypeFlags()
+        .withVariadic(value & Variadic)
+        .withAutoClosure(value & AutoClosure)
+        .withEscaping(value & Escaping)


I don't feel like autoclosure or escaping belong in the parameter flags, but that concern is more-or-less independent of your improvements to function type metadata.

@DougGregor I think I can mask them out from the metadata with ease, do you think the approach here is viable overall?

rjmccall · 2017-10-23T07:06:36Z

include/swift/Runtime/Metadata.h

+  uint32_t getParameterFlags(unsigned index) const {
+    auto numParams = getNumArguments();
+    assert(index < numParams);
+    auto *flags = reinterpret_cast<const uint32_t *>(this + 1 + numParams);


This pointer computation seems very wrong; the + numParams is scaled by sizeof(FunctionTypeMetadata).

Also, I'm not sure I understand why you're being incremental about changing the ABI here, at least as regards the layout of FunctionTypeMetadata. Is there a good reason not to design and implement the desired ABI at this point, since you're revising the ABI anyway? Why force someone else to come by and do all these same updates again?

I will redo the pointer but I don’t understand what do you mean by incremental changes here, can you please elaborate? I am thinking of doing everything related to function metadata ABI, this is just a first changes wrt the layout I wanted input on...

This seems to be implementing a flat, always-present array. The things we talked about:

maybe not representing this as an array at all

maybe only including elements in the array if they have flags

or at least allowing the array to be optional

Every change you make here has to be quite involved in terms of e.g. updating all the runtime functions and the compiler's calls to them and how MetadataReader interprets the runtime structures, so I'm questioning whether it really makes sense to do all these changes in separate patches.

Ah sure, I understand. I'm planning to do all this next as one step and update existing commit, just wanted to make sure that I haven't done something obviously terrible since I'm pretty new to LLVM...

rjmccall · 2017-10-23T07:08:00Z

include/swift/Runtime/Metadata.h

-swift_getFunctionTypeMetadata1(FunctionTypeFlags flags,
-                               const void *arg0,
+swift_getFunctionTypeMetadata1(FunctionTypeFlags flags, const void *arg0,
+                               const void *paramFlags,


If we're no longer mangling things into the low bits of the parameter types, we can just use 'const Metadata *' instead of 'void *', since we're not trying to encourage caution.

Why is paramFlags a void*?

getFunctionTypeMetadataN is a convenience function for common cases. Do we have any reason to think that parameter type flags are a common enough case to justify making these functions less convenient?

You also need to update all the getFooFunctionTypeMetadata functions below this, or at least their most-general cases.

I wasn’t sure what to you for parameter flags here, should it be uint32_t * instead? I think parameter flags for single parameter are pretty common, I’ve seen examples in compatibility suite.

I don't doubt that they occur, just that they occur in enough frequency to warrant adding extra requirements to the common case. Can you get counts here?

I've found 87 occurrences of functions with inout and 1-3 arguments in source compatibility projects, most of them in GRDB project. I'm pretty sure there is going to be much more with __shared when that's available... I'm also curious - if not array of parameters how else should we track flags in this case, with individual arguments?

Do you happen to have the full histogram for the source compatibility suite? Specifically I'd like to know the cross table of (convention) x (argument count) x (has any flags).

I agree that __shared/__owned will make flags more common. (We'll have to decide which is the default, and obviously it should track the language decision because the language decision will make the default much more common than the other.)

Our original decision to go up to 3 was not based on any particular insight that I remember. The purpose of these functions is to optimize code size for common cases, and setting up a call with 7 arguments is already a ton of code, so the benefit might be negligible. Maybe we'll want to provide specializations for (1,false), (2, false), (3, false) but just (1, true). Certainly it seems questionable whether we should do these common-path entrypoints for each of the secondary @conventions; maybe we should just have common-path entrypoints that take the convention as an argument. Anyway, data is the key here.

Oh, are you just grepping for typealiases of function type or something?

Yes, that's exactly what I was doing, I wish we had some tools to generate reports like that but I'm afraid we don't, or maybe I just don't know about them. My thinking was that if there aren't many functions with inout (overall) in the project then and even fewer are going to require metadata generation that says something about distribution...

RE: swift_getFunctionTypeMetadataWithFlags1 - Sure! I got that idea as well, just wasn't sure if you are proposing something even more sophisticated... I think it makes sense because many 1/2/3 parameter functions are likely not to have any flags, judging by compatibility suite. Which brings us to question about having separate "function parameter metadata" which is going to include pointer to type + uint32_t for flags and not going to require any changes to swift_getFunctionTypeMetadata* functions and MetadataCache, WDYT?

RE: MetadataCache - Ah, I see, that make sense to me!

I think having a separate metadata object for each parameter would be rather gratuitously wasteful, and I can't think of anything that would benefit from it.

Not always but only if there are flags present, if that’s not a very common occurance it’s not going to waste too much space, right?

I think there's some value in having the layout of the component types not vary that drastically based on whether flags are present. This is why I was suggesting having separate arrays.

I totally agree! The reason why I bring up this parameter type ref metadata idea is because it makes it much easier to work with function metadata and doesn't require any specific optimizations, like in case of the array of flags, we'd like to keep only flags for parameters which have them, and don't use any additional place, such most likely would be more complicated comparing to extracting a single uint32_t from metadata ref and this approach wouldn't require adding even more optimized calls for 1/2/3 parameter functions with and w/e flags...

xedin · 2017-10-26T06:28:47Z

@rjmccall Can you please take another look? I've updated everything according to our discussion - added new WithFlags methods (and made them used if there are no parameters with flags), fixed parameters to be const Metadata *, plus a bug related to casting. I think the only remaining thing is to fix function metadata cache to respect new flags...

rjmccall · 2017-10-26T18:37:17Z

include/swift/AST/Types.h

+  ParameterTypeFlags withAutoClosure(bool isAutoClosure) const {
+    return ParameterTypeFlags(isAutoClosure
+                                  ? value | ParameterTypeFlags::AutoClosure
+                                  : value - ParameterTypeFlags::AutoClosure);


Is this an actual int subtraction? You can't do that safely unless you're asserting that the flag is already set in 'value'.

This comment is still applicable.

I overlooked that, it's not longer necessary for function metadata anyway, so I'll just remove all of the methods I've added to ParameterTypeFlags.

And I've looked it up - it's actually not a int substitution, but rather an OptionSet operation, so it should be safe to do that.

rjmccall · 2017-10-26T18:39:15Z

include/swift/Remote/MetadataReader.h

          Param.setType(ParamTypeRef);
+          Param.setFlags(ParameterTypeFlags::fromRaw(ParameterFlags));


It is really not okay to assume that the AST's ParameterTypeFlags type matches the runtime-metadata type. Please make these separate types with their own internal representation of all the various flags. I'm sure I asked you to do this before.

Ok, will do.

rjmccall · 2017-10-26T18:40:46Z

include/swift/Runtime/Metadata.h

                               const Metadata *resultMetadata);

 SWIFT_RUNTIME_EXPORT
 const FunctionTypeMetadata *
+swift_getFunctionTypeMetadata1WithFlags(FunctionTypeFlags flags,
+                                        const Metadata *arg0,
+                                        const uint32_t *paramFlags,


This should just take a uint32_t, not a pointer.

Ok, will do.

rjmccall · 2017-10-26T18:41:00Z

include/swift/Runtime/Metadata.h

+swift_getFunctionTypeMetadata2WithFlags(FunctionTypeFlags flags,
+                                        const Metadata *arg0,
+                                        const Metadata *arg1,
+                                        const uint32_t *paramFlags,


This should take two uint32_t's.

If we are going to go this route I going to re-arrange it so it’s param1, flags1, param2, flags2 etc.

Yes, that seems reasonable.

rjmccall · 2017-10-26T18:41:55Z

include/swift/Runtime/Metadata.h

+                                                const Metadata *arg1,
+                                                const Metadata *arg2,
+                                                const uint32_t *paramFlags,
+                                                const Metadata *resultMetadata);


Are you planning to collect that histogram of actual uses and then figure out what flags we really want in a follow-up?

Regardless, you should follow the same pattern here and take three uint32_t's. Only the most general entrypoint should take it indirectly.

rjmccall · 2017-10-26T18:46:47Z

stdlib/public/runtime/Metadata.cpp

+      auto numArguments = getFlags().getNumArguments();
+      assert(index < numArguments);
+      auto *flags = reinterpret_cast<const uint32_t *>(
+          FlagsArgsAndResult[numArguments + 1]);


Wait, are you storing a pointer in the metadata? What is this pointer pointing to? Is the assumption that callers of getFunctionTypeMetadata will permanently allocate a flags array?

Currently I have it created as constant global array and passed as a pointer, I think it makes sense to copy or re-arrange it somehow when stored in the cache, I just didn’t do that part yet...

If I wanted to avoid array allocation and just encode flags inline after each parameter, how should I go avoid storing uint32_t as void*?

I would suggest storing it as two separate arrays in the metadata object, one after the other.

Sorry but I don’t think that I fully understand what you mean, I am talking about void ** we pass to swift_getFunctionTypeMetadata, do you mean the same thing by metadata object? The reason I decided to go with this const array is because it’s easy to pass it in, but I would really rather do it as param1,flags1, etc. here the same way as 1/2/3 functions are going to be. I just don’t know how to encode uint32_t value as an element of void ** ...

Well, you could extend it to intptr_t and then cast, which is easy enough to do in LLVM. But I wasn't thinking about the fact that swift_getFunctionTypeMetadata has such an opaque interface. Frankly, I'm not sure why it does, vs. taking the FunctionTypeFlags and a types (and now a flags) pointer.

So here is how I am thinking of encoding arbitrary parameters since it’s easy to use uint32_t: represent each of the parameter’s metadata as void * with last bit set to 1 if it has flags, 0 otherwise, format is going to be paramN, flagsN?, ... if the last bit is not set consider next pointer in the array to be new parameter, otherwise it’s uint3_t representing flags for current parameter, WDYT?

Do you mean in the interface to swift_getFunctionTypeMetadata or do you mean in the storage in a FunctionTypeMetadata? I tend to think that separate arrays, where the flags array is optional, is a better representation overall for both cases.

In the metadata, we can store whether there are any parameter flags in the FunctionTypeFlags.

I mean as swift_getFunctionTypeMetadata argument, but this might be beneficial for cache too because it avoids storing a bunch of zeros in the array.

xedin · 2017-10-27T23:40:18Z

include/swift/Runtime/Metadata.h

@@ -1760,7 +1758,15 @@ struct TargetFunctionTypeMetadata : public TargetMetadata<Runtime> {
  TargetPointer<Runtime, const Argument> getArguments() const {
    return reinterpret_cast<TargetPointer<Runtime, const Argument>>(this + 1);


@rjmccall I'm working on the changes to separate parameters from flags when stored in the entry of the cache but I need some help understanding how to do that, especially what this + 1 really means here? I've read comment in SimpleGlobalCache which talks about how getExtraAllocationSize is used, so here is what I think I need to do to make it work, please correct me if I'm wrong:

Modify getExtraAllocationSize so it tail-allocates enough space for array of uint32_t equal to number of parameters like this

static size_t getExtraAllocationSize(Key key) { auto numParams = key.getFlags().getNumArguments(); return key.getFlags().getNumArguments() * (sizeof(FunctionTypeMetadata::Argument) + sizeof(uint32_t)); }

Add a method to FunctionCacheEntry:

TargetPointer<Runtime, uint32_t *> getParameterFlags() { return reinterpret_cast<TargetPointer<Runtime, uint32_t *>>(__address here__); }

Should address here be this + (numParams * sizeof(TargetFunctionTypeMetadata::Argument)) + 1 ?

Remember that pointer arithmetic in C is scaled by the size of the pointee type. this + 1 is an idiom used when you allocate extra memory after an object ("extra" meaning "not just sizeof(T)"), because it very conveniently creates a pointer to that extra memory without needing to write out the type of this. You then generally need to cast that pointer to some other type in order to do useful pointer arithmetic within the tail-allocation.

So the address needs to be reinterpret_cast<char*>(this + 1) + (numParams * sizeof(TargetFunctionTypeMetadata::Argument)).

Or, alternatively, reinterpret_cast<Argument*>(this + 1) + numParams.

rjmccall · 2017-10-29T19:08:55Z

include/swift/Remote/MetadataReader.h

@@ -812,22 +813,17 @@ class MetadataReader {
        sizeof(TargetFunctionTypeMetadata<Runtime>);
      for (StoredPointer i = 0; i < Function->getNumArguments(); ++i,
           ArgumentAddress += sizeof(StoredPointer)) {
-        StoredPointer FlaggedArgumentAddress;
+        StoredPointer ParameterAddress;


I would call this ParamMetadata or something like that.

xedin · 2017-10-30T04:58:03Z

@rjmccall Does this look better? It's not yet complete but 98% done I think, I made it so parameters and flags are stored separately and flags are encoded independently from AST and converted when needed.

rjmccall

Yes, I'm much happier with the basic approach here, thank you. We can revisit the question of which specialized getFunctionTypeMetadata functions to provide in a follow-up patch. I've left some comments, and I'm sure I'll have more with the complete patch, but please proceed with this design.

rjmccall · 2017-10-29T19:09:40Z

include/swift/Remote/MetadataReader.h

          return BuiltType();

        FunctionParam<BuiltType> Param;
+        if (auto ParamTypeRef = readTypeFromMetadata(ParameterAddress)) {
+          //const auto ParameterFlags = Function->getParameterFlags()[i];


Okay. Is this not ready for me to review?

rjmccall · 2017-10-30T05:51:49Z

include/swift/ABI/MetadataValues.h

+    ConventionMask = 0x0F000000U,
+    ConventionShift = 24U,
+    ThrowsMask = 0x10000000U,
+    ParamFlagsMask = 0x01000000U,


The indentation before was better because it made it a lot clearer that the bitmasks didn't interfere.

16 million formal arguments is a bit ridiculous; let's drop that to 16 bits, since 65535 arguments is still a rather absurd number to support. We should probably make the convention at least 8 bits, though. That leaves us with 8 bits, of which we've claiming two already. We can always use one of those bits to mean "there are more flags" and then pass those in the arguments array / store them somewhere else in the metadata. Does that seem reasonable?

Sorry, this is what git clang-format does, I've tried to fix indentation of enums where possible but failed in some places still :(

Will do the changes to size of the flags after I'm done with the flags and other changes, if that's ok?

rjmccall · 2017-10-30T05:59:14Z

include/swift/ABI/MetadataValues.h

+    InOutMask    = 0x10000000U,
+    SharedMask   = 0x01000000U,
+    VariadicMask = 0x00100000U,
+  };


I don't know why you're building these from the high bits. All else being the same, it's generally cheaper for the compiler to materialize small integers, not large ones.

I've just looked at some other places, and I didn't know that it's more expensive, will fix, thanks!

It's pretty minor, but yes, it is often the case that you can do something with a very small integer in one instruction that takes multiple instructions with a larger integer, so if it's otherwise an arbitrary choice, prefer to use smaller integers for more important flags.

Understood, thanks!

rjmccall · 2017-10-30T06:03:54Z

include/swift/Runtime/Metadata.h

-
+
+  void setParameterFlags(unsigned index, ParameterFlags flags) {
+    assert(index <= getNumArguments());


Just <, not <=.

I should stop doing coding at night :)

rjmccall · 2017-10-30T06:04:01Z

include/swift/Runtime/Metadata.h

+  }
+
+  ParameterFlags getParameterFlags(unsigned index) const {
+    assert(index <= getNumArguments());


Same thing here.

rjmccall · 2017-10-30T06:08:41Z

include/swift/Remote/MetadataReader.h

+
+      for (StoredPointer i = 0; i < numParameters; ++i,
+                         ArgumentAddress += sizeof(StoredPointer),
+                         ParameterFlagsAddress += sizeof(StoredPointer)) {


I believe you need to be adding sizeof(uint32_t) here.

But in general, none of this should be necessary, since it's not reading memory outside of the true bounds of the FunctionTypeMetadata. You just need to update the Function case in readMetadata so that it computes the true bounds correctly, just like how it's done for Tuples. If you do that, you should be able to just use the normal access functions on TargetFunctionTypeMetadata (as long as they don't assert anything you haven't already checked!).

I think I understand now what you are talking about - it's done for parameters this way because we need to read a point to metadata itself from the argument position, but for flags we don't have to do that because they are values themselves. Thanks again!

rjmccall · 2017-10-30T06:10:50Z

include/swift/Runtime/Metadata.h

-                               const Metadata *resultMetadata);
+swift_getFunctionTypeMetadata2WithFlags(FunctionTypeFlags flags,
+                                        const Metadata *arg0,
+                                        const uint32_t flags0,


Don't const-qualify a parameter like this. Also, this should be ParameterTypeFlags, right?

I think it's fine to make it ParameterFlags for 1/2/3 functions but should it be const ParameterFlags * for the general case too?

The way I see is it that the use of a target-specific type is specifically just for passing these values as direct arguments to the runtime functions. Arguably that implies that we should make the enums ({Function,Parameter}TypeFlags) non-templated and just make the runtime functions take a size_t with a comment about what enum they really are. The place where we take an array of them by reference should definitely use a pointer to a 32-bit type.

Alright, this means that my changes are correct :)

rjmccall · 2017-10-30T06:15:59Z

include/swift/ABI/MetadataValues.h

@@ -606,6 +615,55 @@ class TargetFunctionTypeFlags {
 };
 using FunctionTypeFlags = TargetFunctionTypeFlags<size_t>;

+template <typename int_type> class TargetParameterTypeFlags {


We seem to be assuming that the backing storage for this is always a uint32_t. I think it's still maybe a good idea to have it be templated, because it solves a lot of annoying calling-convention portability concerns to always pass a parameter as a full word instead of a smaller integer, but I think it merits a comment about why we're doing it.

xedin · 2017-10-31T03:29:05Z

include/swift/Runtime/Metadata.h


  TargetFunctionTypeFlags<StoredSize> Flags;
+  TargetPointer<Runtime, const uint32_t> ParamFlags;


@rjmccall Just wanted to double check if that's what you were talking about when referring to how tuple metadata is done?

xedin · 2017-10-31T07:26:20Z

Looks like after I've added new WithFlags methods I've broke something related to Objective-C but I have no clue how to debug that, if I define function with parameter flags now and try to run the code process is crashing with:

0  swift                    0x000000010ab5af7c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  swift                    0x000000010ab5b579 PrintStackTraceSignalHandler(void*) + 25
2  swift                    0x000000010ab56d39 llvm::sys::RunSignalHandlers() + 425
3  swift                    0x000000010ab5b9d2 SignalHandler(int) + 354
4  libsystem_platform.dylib 0x00007fff613e4f5a _sigtramp + 26
5  libsystem_platform.dylib 0x000000011d631b5a _sigtramp + 3156528154
6  libswiftCore.dylib       0x0000000117319c45 swift::metadataimpl::ObjCRetainableBox::retain(void*) + 21
7  libswiftCore.dylib       0x0000000117319c1c swift::metadataimpl::RetainableBoxBase<swift::metadataimpl::ObjCRetainableBox, void*>::initializeWithCopy(void**, void**) + 28
8  libswiftCore.dylib       0x00000001173179c4 swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*) + 36
9  libswiftCore.dylib       0x0000000117317960 swift::metadataimpl::BufferValueWitnesses<swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>, 8ul, 8ul, (swift::metadataimpl::FixedPacking)1>::initializeBufferWithCopyOfBuffer(swift::ValueBuffer*, swift::ValueBuffer*, swift::TargetMetadata<swift::InProcess> const*) + 48
10 libswiftCore.dylib       0x0000000116b84194 swift::metadataimpl::BufferValueWitnesses<swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>, 8ul, 8ul, (swift::metadataimpl::FixedPacking)1>::initializeBufferWithCopyOfBuffer(swift::ValueBuffer*, swift::ValueBuffer*, swift::TargetMetadata<swift::InProcess> const*) + 4287023204
11 swift                    0x00000001047578de llvm::MCJIT::runFunction(llvm::Function*, llvm::ArrayRef<llvm::GenericValue>) + 1358
12 swift                    0x0000000104763da2 llvm::ExecutionEngine::runFunctionAsMain(llvm::Function*, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, char const* const*) + 3650
13 swift                    0x00000001030307b7 swift::RunImmediately(swift::CompilerInstance&, std::__1::vector<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, swift::IRGenOptions&, swift::SILOptions const&) + 7271
14 swift                    0x0000000102f95fea performCompile(swift::CompilerInstance&, swift::CompilerInvocation&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*, swift::UnifiedStatsReporter*) + 25866
15 swift                    0x0000000102f8d5b2 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 8882
16 swift                    0x0000000102eb9573 main + 4819
17 libdyld.dylib            0x00007fff61164145 start + 1

@rjmccall Any ideas how should I approach this?

rjmccall · 2017-10-31T07:50:03Z

include/swift/Remote/MetadataReader.h

+      auto flags = FunctionTypeFlags()
+                       .withConvention(Function->getConvention())
+                       .withThrows(Function->throws())
+                       .withParameterFlags(Function->hasParameterFlags());
      auto BuiltFunction =
          Builder.createFunctionType(Parameters, Result, flags);


I think you'll need to change the createFunctionType API to also take parameter flags, which probably means remapping them to the AST flags.

Also, what I mean about TupleMetadata is that you should modify the MetadataKind::Function case in readMetadata to start by reading the FunctionTypeMetadata and then use that to figure out the complete size to read, like the case for MetadataKind::Tuple does.

Ah ok, I think I understand know, I thought you meant treat flags like labels are treated in case of tuple...

Regarding createFunctionType API - each of the elements in Parameters already has flags attached to it, so I think flags are covered that way?...

Oh, yes, I see, that makes sense.

rjmccall · 2017-10-31T07:54:03Z

I would guess that your bug is that you're scribbling out of bounds of the metadata object somehow.

xedin · 2017-11-06T01:30:08Z

@rjmccall I think I've made all of the changes required to make it work, please take a look one more time, thanks!

Currently only single 'inout' flag has been encoded into function metadata, these changes extend function metadata to support up to 32 flags per parameter.

…rences

…gs arguments Switch most general endpoint to be `flags, parameters, parameterFlags, result`, instead of opaque `void **`, more specialized ones to use follow argument scheme: `flags, param0, [flags0], ..., paramN, [flagsN], result` and store parameter/flags information separately in `FunctionCacheEntry::{Key, Data}` as well.

…tadata use

xedin · 2017-11-06T19:18:19Z

@swift-ci please test

rjmccall

Okay, in addition to the change I request above, it looks like you're still on the hook for two things:

Changing the layout and size of the different flags as discussed.
Coming up with good data for which convenience accessors we should actually provide.

But it's fine to handle those in follow-ups.

rjmccall · 2017-11-06T19:23:30Z

include/swift/AST/Types.h

+  ParameterTypeFlags withAutoClosure(bool isAutoClosure) const {
+    return ParameterTypeFlags(isAutoClosure
+                                  ? value | ParameterTypeFlags::AutoClosure
+                                  : value - ParameterTypeFlags::AutoClosure);


This comment is still applicable.

xedin · 2017-11-06T19:32:55Z

@rjmccall Yes, I was thinking to do that after this changes are merged as a follow up PR at least for function flags.

xedin · 2017-11-06T21:40:25Z

lib/IRGen/GenMeta.cpp

-            Address argPtr = IGF.Builder.CreateStructGEP(buffer, i + 1,
+
+          ConstantInitBuilder paramFlags(IGF.IGM);
+          auto flagsArr = paramFlags.beginArray();


@rjmccall One last question I have is if we should keep flags as constant array of stack allocated one? After digging into all this more it seems like we might better off with stack allocated array flags the same way as for parameters because we don't keep them around that long anyway, WDYT?

It is almost certainly less overall binary size to use a const global array than to create one fresh on the stack.

Ok, sounds good, I'll leave it as is then! Thanks again!

xedin · 2017-11-06T23:38:59Z

@swift-ci please test

xedin · 2017-11-07T02:13:27Z

@swift-ci please test source compatibility

rjmccall · 2017-11-07T04:00:47Z

Congrats!

xedin · 2017-11-07T04:04:44Z

Thanks, John! I will make couple of follow-up PRs as we discussed.

xedin · 2017-11-07T08:52:38Z

@rjmccall looks like it was too early to celebrate since this changes broke LLDB :( Do you have any idea where should I start in that project to fix it?

rjmccall · 2017-11-07T08:56:52Z

You almost certainly don't need to modify anything in LLDB; instead, there's probably something broken in the RemoteAST code, i.e. MetadataReader. I would look at the test case, try to figure out what function type metadata is being parsed, and then try to duplicate that failure in the remoteast test harness.

xedin · 2017-11-07T08:59:13Z

@rjmccall Could be it as simple as lldb requiring a clean build instead of incremental to resolve this?

xedin · 2017-11-07T09:25:52Z

I've tried using swift-remoteast-test with printType as in one of the remoteAST tests to print "f" variable and it does print it correctly as (Int) -> Int from metadata...

xedin requested review from rjmccall and DougGregor October 18, 2017 22:21

xedin force-pushed the fn-metadata-changes branch from 179768f to a39658b Compare October 18, 2017 22:28

DougGregor reviewed Oct 20, 2017

View reviewed changes

rjmccall requested changes Oct 23, 2017

View reviewed changes

xedin force-pushed the fn-metadata-changes branch from a39658b to d0c7240 Compare October 26, 2017 06:24

rjmccall reviewed Oct 26, 2017

View reviewed changes

xedin commented Oct 27, 2017

View reviewed changes

rjmccall reviewed Oct 29, 2017

View reviewed changes

xedin force-pushed the fn-metadata-changes branch from ee44e65 to fb3d757 Compare October 30, 2017 04:56

rjmccall reviewed Oct 30, 2017

View reviewed changes

xedin force-pushed the fn-metadata-changes branch 3 times, most recently from 2f3a021 to be79e95 Compare October 30, 2017 08:26

rjmccall approved these changes Oct 30, 2017

View reviewed changes

xedin force-pushed the fn-metadata-changes branch from be79e95 to f5b0699 Compare October 31, 2017 03:27

xedin commented Oct 31, 2017

View reviewed changes

rjmccall reviewed Oct 31, 2017

View reviewed changes

xedin force-pushed the fn-metadata-changes branch from 21d51b6 to 9e67482 Compare November 4, 2017 08:52

xedin mentioned this pull request Nov 5, 2017

[RemoteAST] Fix metadata reader to properly read tuple type metadata #12766

Merged

xedin force-pushed the fn-metadata-changes branch 2 times, most recently from 89bb61c to 191f1e7 Compare November 6, 2017 01:28

apple deleted a comment from swift-ci Nov 6, 2017

xedin added 7 commits November 6, 2017 11:16

[IRGen] Add parameter flags to function type metadata

b33a6c7

Currently only single 'inout' flag has been encoded into function metadata, these changes extend function metadata to support up to 32 flags per parameter.

[IRGen] Switch function metdata to use Metdata * for parameter refe…

99f188e

…rences

[ABI/IRGen] Add custom function parameter flags representation for me…

f6b0d2d

…tadata use

[ABI/Runtime] NFC: Rename function metadata arguments to parameters

801b35e

[RemoteAST] Fix metadata reader to properly read function metadata

e80455e

[Casting] NFC: add some more tests for functions with parameter flags

0685106

xedin force-pushed the fn-metadata-changes branch from 119a3fa to 0685106 Compare November 6, 2017 19:17

apple deleted a comment from swift-ci Nov 6, 2017

rjmccall reviewed Nov 6, 2017

View reviewed changes

apple deleted a comment from swift-ci Nov 6, 2017

[AST] NFC: Remote unused accessor methods from ParameterTypeFlags

fbd502a

xedin commented Nov 6, 2017

View reviewed changes

apple deleted a comment from swift-ci Nov 6, 2017

apple deleted a comment from swift-ci Nov 7, 2017

xedin changed the title ~~[WIP] [IRGen] Improvements to function type metadata~~ [IRGen] Improvements to function type metadata Nov 7, 2017

xedin merged commit e23a6f1 into apple:master Nov 7, 2017

		Param.setType(ParamTypeRef);
		Param.setFlags(ParameterTypeFlags::fromRaw(ParameterFlags));

		@@ -1760,7 +1758,15 @@ struct TargetFunctionTypeMetadata : public TargetMetadata<Runtime> {
		TargetPointer<Runtime, const Argument> getArguments() const {
		return reinterpret_cast<TargetPointer<Runtime, const Argument>>(this + 1);



		void setParameterFlags(unsigned index, ParameterFlags flags) {
		assert(index <= getNumArguments());


		TargetFunctionTypeFlags<StoredSize> Flags;
		TargetPointer<Runtime, const uint32_t> ParamFlags;

[IRGen] Improvements to function type metadata #12500

[IRGen] Improvements to function type metadata #12500

Conversation

xedin commented Oct 18, 2017 • edited

xedin commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall Oct 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin commented Oct 30, 2017

rjmccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xedin commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmccall commented Oct 31, 2017

xedin commented Oct 18, 2017 •

edited

xedin Oct 26, 2017 •

edited

rjmccall Oct 28, 2017 •

edited