[DNM] compute the field index with a request instead of caching it in the instruction #37831

eeckstein · 2021-06-08T16:22:05Z

This is just a prototype.

…nstruction

xedin

Mostly looks good to me, there is a mismatch between std::tuple and void in the declaration.

xedin · 2021-06-08T16:55:32Z

include/swift/AST/Decl.h

  NamedPattern *NamingPattern = nullptr;

+  int fieldIndex = -1;


It's unfortunate that we'd still need to cache index in the decl but it would be expensive to compute indexes with request for an individual property since there could be quite a few stored properties...

xedin · 2021-06-08T16:56:30Z

include/swift/AST/SILOptimizerRequests.h

@@ -87,6 +87,20 @@ class LoweredSILRequest
                                      ASTLoweringDescriptor desc) const;
 };

+class ComputeFieldIndicesRequest :
+    public SimpleRequest<ComputeFieldIndicesRequest,
+                         std::tuple<>(NominalTypeDecl *),


I think this has to be void?

IIRC the evaluator doesn't currently support void return types, we usually use evaluator::SideEffect for that (which is an alias for std::tuple<>)

Ah interesting, since the definition accepted void I got confused...

Yeah I believe the signature written in the SWIFT_REQUEST macro isn't actually used for anything, so isn't currently checked for consistency 😬

xedin · 2021-06-08T16:56:38Z

include/swift/AST/SILOptimizerRequests.h

+  friend SimpleRequest;
+
+  // Evaluation.
+  std::tuple<> evaluate(Evaluator &evaluator, NominalTypeDecl *decl) const;


Here as well?

xedin · 2021-06-08T16:57:02Z

include/swift/AST/SILOptimizerTypeIDZone.def

@@ -20,3 +20,6 @@ SWIFT_REQUEST(SILOptimizer, ExecuteSILPipelineRequest,
 SWIFT_REQUEST(SILOptimizer, LoweredSILRequest,
              std::unique_ptr<SILModule>(ASTLoweringDescriptor),
              Uncached, NoLocationInfo)
+SWIFT_REQUEST(SILOptimizer, ComputeFieldIndicesRequest,
+              void(NominalTypeDecl *),
+              Cached, NoLocationInfo)


I think you can make it Uncached since it doesn't return anything?

hamishknight · 2021-06-08T17:08:29Z

include/swift/AST/Decl.h

@@ -4821,6 +4825,8 @@ class VarDecl : public AbstractStorageDecl {
    return hasName() ? getBaseIdentifier().str() : "_";
  }

+  int getFieldIndex() const;


I feel like this should probably have a slightly scarier name to convey it might entail a non-trivial amount of work (such as synthesising backing storage vars) – something like getABIStoredPropertyIndex perhaps? Or even just getABIFieldIndex? I'd be interested to hear what others think.

hamishknight · 2021-06-08T17:13:14Z

include/swift/AST/Decl.h

  NamedPattern *NamingPattern = nullptr;

+  int fieldIndex = -1;


This used to be cached as an unsigned, are there any issues here in halving the range of values it can store?

It might also be worth moving into the inline bitfield for VarDecl, though that would mean you'd likely have to add a private getter and setter for the cached value so the request can access it.

hamishknight · 2021-06-08T17:13:38Z

include/swift/AST/Decl.h

@@ -4788,8 +4788,12 @@ enum class PropertyWrapperSynthesizedPropertyKind {
 /// VarDecl - 'var' and 'let' declarations.
 class VarDecl : public AbstractStorageDecl {
  friend class NamingPatternRequest;
+  friend class GetFieldIndexRequest;


GetFieldIndexRequest doesn't appear to be defined

good catch. I'll remove it

hamishknight · 2021-06-08T17:16:27Z

lib/AST/Decl.cpp

+  if (fieldIndex < 0) {
+    NominalTypeDecl *parent = getDeclContext()->getSelfNominalTypeDecl();
+    evaluateOrDefault(getASTContext().evaluator,
+                      ComputeFieldIndicesRequest{parent}, {});
+    assert(fieldIndex >= 0);
+  }


We generally want to try and avoid having side-effecting requests like this. Ideally ComputeFieldIndicesRequest would be a SeparatelyCached cached request, and get and set the cached value in its getCachedResult/cacheResult methods.

You could also fill in the other field indices for the other properties as you iterate by calling cacheOutput on the evaluator for those properties, similar to what DefaultArgumentInitContextRequest does.

Hm, would a better approach here be to remove field from the decl and just store everything in a cached DenseMap instead? That would use more memory but at least wouldn't have any side-effects.

If it's going into a DenseMap then that would basically be more or less equivalent to just using the evaluator's internal cache, which we could possibly do here.

Well, I could also add a DenseMap for this in SILModule without using the evaluator machinery. But the field indices are something which are used in a critical path in the optimizer (for computing projections). So the purpose of this exercise is to avoid the overhead of a hash lookup.

Ideally, the index of all stored properties should be computed and cached for a type during a single request. This should result in a unique index for each property in an object. This is something that we can't currently rely on but would greatly simplify analysis because the index would have meaning independent of the address type.
@eeckstein is right that the index should be directly loadable from an instruction without any hash lookup. That was the point of the instruction cache, although I'm happy to see it go.

hamishknight · 2021-06-08T17:19:01Z

include/swift/AST/SILOptimizerTypeIDZone.def

@@ -20,3 +20,6 @@ SWIFT_REQUEST(SILOptimizer, ExecuteSILPipelineRequest,
 SWIFT_REQUEST(SILOptimizer, LoweredSILRequest,
              std::unique_ptr<SILModule>(ASTLoweringDescriptor),
              Uncached, NoLocationInfo)
+SWIFT_REQUEST(SILOptimizer, ComputeFieldIndicesRequest,


Hmm, ideally we'd have a SIL request evaluator zone for this. I think for now at least it might be better off placed in the TypeChecker zone, as it shouldn't depend on anything outside of Sema.

ok. I can move it to the type checker

eeckstein · 2022-05-16T22:46:33Z

Closing in favor of #58933

compute the field index with a request instead of caching it in the i…

7cb75b0

…nstruction

eeckstein requested review from slavapestov and xedin June 8, 2021 16:24

xedin reviewed Jun 8, 2021

View reviewed changes

hamishknight reviewed Jun 8, 2021

View reviewed changes

eeckstein closed this May 16, 2022

[DNM] compute the field index with a request instead of caching it in the instruction #37831

[DNM] compute the field index with a request instead of caching it in the instruction #37831

Uh oh!

Conversation

eeckstein commented Jun 8, 2021

Uh oh!

xedin left a comment

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!

hamishknight Jun 8, 2021 • 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!

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!

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!

eeckstein commented May 16, 2022

Uh oh!

Uh oh!

hamishknight Jun 8, 2021 •

edited

Loading