Builtin.FixedArray #76831

jckarter · 2024-10-02T19:01:51Z

Add a primitive that can be used to represent uniform aggregates of contiguously-laid-out values, parameterized by a generic size and element type. Builtin.FixedArray gets all of its layout properties from its element type, and copies/moves/destroys elementwise.

Azoy · 2024-10-15T18:49:50Z

lib/IRGen/GenBuiltin.cpp

+      elements.push_back(Element.buildTypeLayoutEntry(IGM, eltTy,
+                                                      useStructLayouts));
+    });
+    return IGM.typeLayoutCache.getOrCreateAlignedGroupEntry(elements, T,


You should be able to use the new ArrayLayoutEntry which takes an element layout, element type, and a count type (which is trivial to build back from an unsigned IntegerType::get(std::to_string(arraySize), false, ctx)).

Azoy · 2024-10-15T18:51:45Z

lib/IRGen/GenBuiltin.cpp

+  }
+
+  template<typename...Addresses, typename Body>
+  void eachElementAddrLoop(IRGenFunction &IGF,


Not sure how useful it'd be here, but I added IRGenFunction::emitLoopOverElements to support the ArrayLayoutEntry work.

Ah, I see! I'll work on reconciling these in a follow up since it would be good to have only one implementation. emitLoopOverElements looks like it has some bugs in its ad-hoc element indexing that could be avoided by using TypeInfo::indexArray.

jckarter · 2024-10-15T21:04:01Z

@swift-ci Please test

lib/IRGen/GenBuiltin.cpp

jckarter · 2024-10-16T03:35:21Z

@swift-ci Please test

jckarter · 2024-10-16T14:42:58Z

@swift-ci Please test

jckarter · 2024-10-16T16:35:47Z

@swift-ci Please test

jckarter · 2024-10-16T21:09:02Z

@swift-ci Please test

jckarter · 2024-10-16T23:16:55Z

@swift-ci Please test

include/swift/AST/Types.h

jckarter · 2024-10-17T15:14:35Z

@swift-ci Please test

jckarter · 2024-10-17T19:06:25Z

@swift-ci Please test

rjmccall · 2024-10-17T23:23:30Z

include/swift/ABI/Metadata.h

+  StoredPointerDifference Count;
+  ConstTargetMetadataPointer<Runtime, swift::TargetMetadata> Element;
+
+  StoredPointerDifference getCount() const {


I'm a little concerned about callers naively thinking this is going to return the raw value, and then they won't be correctly round-tripping the metadata. Can we give this a name that makes it clear that it's a zero-bounded count?

rjmccall · 2024-10-17T23:36:19Z

include/swift/AST/Types.h

+  CanType getSize() const { return Size; }
+
+  /// Get the fixed integer number of elements if known and zero or greater.
+  std::optional<unsigned> getFixedSize() const;


Please use a typedef for the value type here, and it should probably be represented with uint64_t.

Fair point about uint64_t, though I don't know that a typedef would do much over just using uint64_t directly.

I think it's communicatively valuable. I guess a 128-bit target is unlikely, though, you're right.

rjmccall · 2024-10-17T23:39:03Z

lib/IRGen/GenBuiltin.cpp

@@ -1537,3 +1539,682 @@ void irgen::emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &Builtin,

  llvm_unreachable("IRGen unimplemented for this builtin!");
 }
+
+template<typename BaseTypeInfo, typename ElementTypeInfo = BaseTypeInfo>
+class ArrayTypeInfoBase : public BaseTypeInfo {


Please put all of this in a new file.

rjmccall · 2024-10-17T23:41:37Z

lib/IRGen/GenBuiltin.cpp

+  virtual llvm::Value *getArraySize(IRGenFunction &IGF, SILType T) const = 0;
+  virtual std::optional<unsigned> getFixedArraySize(SILType T) const = 0;
+
+  template<typename Body>


Please either add a comment explaining the expected signature of body or just make this use llvm::function_ref.

rjmccall · 2024-10-17T23:41:54Z

lib/IRGen/GenBuiltin.cpp

+    auto predBB = IGF.Builder.GetInsertBlock();
+
+    auto loopBB = IGF.createBasicBlock("each_vector_element");
+    auto endBB = IGF.createBasicBlock("end_vector_element");


array, please

rjmccall · 2024-10-17T23:59:33Z

lib/IRGen/GenBuiltin.cpp

+  buildTypeLayoutEntry(IRGenModule &IGM,
+                       SILType T,
+                       bool useStructLayouts) const override {
+    std::vector<TypeLayoutEntry *> elements;


This is dead, which seems wrong.

It's not dead. This is necessary to override the pure virtual requirement of FixedArrayTypeInfo (note the override).

Sorry, I meant the elements variable.

lib/IRGen/GenBuiltin.cpp

rjmccall · 2024-10-18T00:02:13Z

lib/IRGen/GenBuiltin.h

@@ -35,7 +35,7 @@ namespace irgen {
  void emitBuiltinCall(IRGenFunction &IGF, const BuiltinInfo &builtin,
                       BuiltinInst *Inst, ArrayRef<SILType> argTypes,
                       Explosion &args, Explosion &result);
-
+                       


rjmccall · 2024-10-18T00:04:51Z

lib/IRGen/GenBuiltin.cpp

+  }
+
+  template<typename Body>
+  void eachElementOffset(Body &&body) const {


Either comment the signature or use llvm::function_ref; also, this should definitely be a uint64_t.

rjmccall · 2024-10-18T00:06:57Z

lib/IRGen/GenBuiltin.cpp

+
+  void addToAggLowering(IRGenModule &IGM, SwiftAggLowering &lowering,
+                        Size offset) const override {
+    eachElementOffset([&](unsigned eltByteOffset){


At what point do you check that offset * elementSize doesn't overflow? And actually, the same question applies to the dynamic cases, right?

Hmm, we don't anywhere else we do type layout, but you're right we probably should here since it's a lot easier to induce a size overflow now.

It probably doesn't make any sense to treat huge arrays as loadable at any level, so I put a cap at 4096 after which we lower the type as address-only in SIL, and Fixed but not Loadable in IRGen.

For dynamic cases, I'm wondering if it'd be sufficient to have the standard library type do a precondition check that the value wouldn't be larger than address space before attempting initialization. If you can never construct such a value to begin with, then the value witness operations should never occur. Though I suppose it would be possible to have a type like

struct Foo<let N: Int> { var buffer: Vector<N, Int> var somethingElse: Int init() { self.somethingElse = 42 self.buffer = initialBuffer() } }

and if an attacker got control of N, they could trick the init() call into writing 42 somewhere bad before the self.buffer initializer got a chance to trap. Maybe we could cap the value witness table size of these array types at some practical limit (maybe 2^31-1 for 32-bit platforms, and 2^55-1 on 64-bit platforms), so that if a huge array shows up as part of an aggregate, the offsets for following fields have some cushion before they wrap around?

I guess we should probably step back and get this right in general. Semantically, we should really be checking legality in the initializer of every type, and it's just that fixed-size types can emit that check statically, hopefully by removing it. Fixed-size arrays obviously make this much easier to violate, but it's always been the right rule. And fixed-size arrays make it easier to violate in all cases, not just when the size of the fixed-size array type itself overflows. So I guess there should be a flag in the VWT for illegal types, and struct/enum/class dynamic layout needs to detect this and set the flag appropriately.

If the semantics are that illegal types are well-formed but invalid to instantiate, ideally they should have zero size. That might screw up extra-inhabitant enum layout, though, since it would make the entire case go away. Do we ever use extra-inhabitant layout strategies based on non-fixed enum cases, e.g. by knowing that the payload is a tuple whose first element has a fixed layout?

If the semantics are that illegal types are well-formed but invalid to instantiate, ideally they should have zero size. That might screw up extra-inhabitant enum layout, though, since it would make the entire case go away. Do we ever use extra-inhabitant layout strategies based on non-fixed enum cases, e.g. by knowing that the payload is a tuple whose first element has a fixed layout?

Yeah, for tuples and structs, we pick the element with the most extra inhabitants to provide extra inhabitants for the aggregate.

If we can properly track type legality, and make such types treated as zero-size, that would have the nice theoretical benefit that an illegal type could be substituted into an enum payload while still allowing the valid payloads of the enum to be practically usable. I wonder if we could retroactively track Never as being illegal too; that might "break" the size we expect of structs with unconditional Never fields, but those structs couldn't actually be instantiated…

Yeah, I think we can make this work, we just have to find a rule that's consistent with our current assumptions about type layout. In particular, consider a type like this:

struct A<T> { var x: SomeClassReference var y: T }

When we're laying out Optional<A<T>> generically, do we always assume A<T> has extra inhabitants because we can see that x does, such that e.g. constructing .none can just write a null pointer (as the first XI) into that field? If so, then the illegal-type layout rule probably has to stay consistent with that — whatever fields are assumed to always exist have to continue to exist, and it's only the other fields that can be removed when we recognize an illegal type.

Ultimately, I think we're deciding between three rules here:

Product types with any illegal fields are zero-sized overall.

All fields following illegal fields in product types are zero-sized.

Product types just treat illegal fields as zero-sized but don't otherwise change layout.

(1) only works if we don't make any XI assumptions about known fields when laying out enums of generic non-fixed product types. (2) only works if those XI assumptions are only made about in the fixed-size prefix of the product. (3) should work in any case, but we might be in deep trouble if illegality starts in the product itself (i.e. all of the fields are legal, but the product layout overflows).

When we're laying out Optional<A> generically, do we always assume A has extra inhabitants because we can see that x does, such that e.g. constructing .none can just write a null pointer (as the first XI) into that field? If so, then the illegal-type layout rule probably has to stay consistent with that — whatever fields are assumed to always exist have to continue to exist, and it's only the other fields that can be removed when we recognize an illegal type.

It looks like dynamic type layout for A dynamically picks the field with the most extra inhabitants:

// We have extra inhabitants if any element does. Use the field with the most. unsigned extraInhabitantCount = 0; for (unsigned i = 0; i < numFields; ++i) { unsigned fieldExtraInhabitantCount = fieldTypes[i]->getNumExtraInhabitants(); if (fieldExtraInhabitantCount > extraInhabitantCount) { extraInhabitantCount = fieldExtraInhabitantCount; } }

So we should be able to make it so that a product with an illegal field itself becomes fully illegal.

jckarter · 2024-10-19T00:15:04Z

@swift-ci Please test

lorentey · 2024-10-19T00:47:17Z

What happened to the builtin's name?

jckarter · 2024-10-19T00:52:25Z

@lorentey We can't call the compiler type BuiltinVectorType because that's already taken for the type representing Builtin.Vec. Several people noted they would rather have the source-level name of the builtin match the compiler-level type name, and it seems like it doesn't really matter that much since you're the only person who has to see it. The standard library can still call its type whatever it wants.

lorentey · 2024-10-19T01:26:55Z

@jckarter If you say so. The new builtin is clearly a close relative of Builtin.Vec<N>x<T>; it seems weird to invent a radically new name for it. (And FixedArray is a curious choice.) The compiler indeed does have a venerable tradition of intentionally using radically different language than the stdlib; I don't think that helps communication, but who am I to question tradition. 😛

jckarter · 2024-10-21T23:17:29Z

@swift-ci Please test

jckarter · 2024-10-22T15:40:06Z

@swift-ci Please test Windows

jckarter · 2024-10-22T18:01:36Z

@swift-ci Please test

rjmccall

One request, but otherwise LGTM

lib/IRGen/GenArray.cpp

jckarter · 2024-10-22T21:40:48Z

@swift-ci Please test

Azoy · 2024-10-22T21:50:15Z

test/abi/Inputs/macOS/arm64/stdlib/baseline

@@ -13625,6 +13625,7 @@ _swift_getExtendedExistentialTypeMetadata
 _swift_getExtendedExistentialTypeMetadata_unique
 _swift_getExtendedExistentialTypeShape
 _swift_getExtendedFunctionTypeMetadata
+_swift_getFixedArrayTypeMetadata


For these tests, you should add an Added: _swift_getFixedArrayTypeMetadata entry to both abi/macOS/arm64/stdlib.swift and abi/macOS/x86_64/stdlib.swift

`Builtin.FixedArray<let N: Int, T: ~Copyable & ~Escapable>` has the layout of `N` elements of type `T` laid out sequentially in memory (with the tail padding of every element occupied by the array). This provides a primitive on which the standard library `Vector` type can be built.

jckarter · 2024-10-22T23:22:38Z

@swift-ci Please test

jckarter force-pushed the builtin-vector branch 3 times, most recently from 71f100b to f481919 Compare October 9, 2024 02:38

Azoy reviewed Oct 15, 2024

View reviewed changes

jckarter force-pushed the builtin-vector branch from f481919 to b9be572 Compare October 15, 2024 21:03

Azoy reviewed Oct 15, 2024

View reviewed changes

lib/IRGen/GenBuiltin.cpp Outdated Show resolved Hide resolved

jckarter force-pushed the builtin-vector branch from b9be572 to e6a5bad Compare October 16, 2024 03:35

jckarter force-pushed the builtin-vector branch from e6a5bad to bcd83d8 Compare October 16, 2024 14:42

jckarter force-pushed the builtin-vector branch from bcd83d8 to 0a54f81 Compare October 16, 2024 16:35

jckarter force-pushed the builtin-vector branch from 0a54f81 to 1970775 Compare October 16, 2024 20:53

jckarter force-pushed the builtin-vector branch from 1970775 to a27bc12 Compare October 16, 2024 23:15

jckarter marked this pull request as ready for review October 17, 2024 00:15

jckarter requested review from a team, hborla, slavapestov, xedin, eeckstein, rjmccall, mikeash, al45tair, xymus, AnthonyLatsis and adrian-prantl as code owners October 17, 2024 00:15

jckarter requested a review from tshortli as a code owner October 17, 2024 00:15

jckarter changed the title ~~[wip] Builtin.Vector~~ Builtin.Vector Oct 17, 2024

eeckstein reviewed Oct 17, 2024

View reviewed changes

include/swift/AST/Types.h Show resolved Hide resolved

jckarter force-pushed the builtin-vector branch from a27bc12 to abee819 Compare October 17, 2024 15:14

jckarter force-pushed the builtin-vector branch from abee819 to a1416ee Compare October 17, 2024 19:05

jckarter changed the title ~~Builtin.Vector~~ Builtin.FixedArray Oct 17, 2024

rjmccall reviewed Oct 18, 2024

View reviewed changes

jckarter force-pushed the builtin-vector branch from a1416ee to a587a0f Compare October 19, 2024 00:15

jckarter force-pushed the builtin-vector branch from a587a0f to f81a6bd Compare October 21, 2024 23:17

jckarter force-pushed the builtin-vector branch from f81a6bd to bc5465c Compare October 22, 2024 18:01

rjmccall reviewed Oct 22, 2024

View reviewed changes

lib/IRGen/GenArray.cpp Outdated Show resolved Hide resolved

jckarter force-pushed the builtin-vector branch from bc5465c to 95de1d2 Compare October 22, 2024 21:40

Azoy reviewed Oct 22, 2024

View reviewed changes

jckarter force-pushed the builtin-vector branch from 95de1d2 to a184782 Compare October 22, 2024 23:22

jckarter merged commit 8242110 into swiftlang:main Oct 23, 2024
5 checks passed

MaxDesiatov mentioned this pull request Oct 24, 2024

Revert "Builtin.FixedArray" #77201

Closed

Builtin.FixedArray #76831

Builtin.FixedArray #76831

Uh oh!

Conversation

jckarter commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Oct 15, 2024

Uh oh!

Uh oh!

jckarter commented Oct 16, 2024

Uh oh!

jckarter commented Oct 16, 2024

Uh oh!

jckarter commented Oct 16, 2024

Uh oh!

jckarter commented Oct 16, 2024

Uh oh!

jckarter commented Oct 16, 2024

Uh oh!

Uh oh!

jckarter commented Oct 17, 2024

Uh oh!

jckarter commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmccall Oct 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Oct 19, 2024

Uh oh!

lorentey commented Oct 19, 2024

Uh oh!

jckarter commented Oct 19, 2024

Uh oh!

jckarter commented Oct 2, 2024 •

edited

Loading

jckarter Oct 18, 2024 •

edited

Loading

rjmccall Oct 20, 2024 •

edited

Loading