Propagate concrete types of arguments to apply #17370

rajbarik · 2018-06-20T20:13:12Z

Final Part of #13991 that propagates concrete types of arguments to apply. This handles non-self arguments as well.

rajbarik · 2018-06-20T20:20:44Z

@slavapestov @atrick This PR is part of #13991 and just focuses on concrete type propagation for all arguments of an Apply.

atrick

New SILCombine-like optimizations should have SIL level unit tests. Those unit tests should cover any special cases that pass the SIL verifier but aren't currently generated with source-level tests.

Much of the code that you're adding is handled by ConcreteExistentialInfo from
#17315. I'd prefer these correctness fixes be merged first, with additional optimization expressed in terms of what I think is now a correct API.

atrick · 2018-06-20T21:25:06Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+              return ConcreteType;
+            return type;
+          },
+          LookUpConformanceInSignature(*(FnTy->getGenericSignature())));


We're not supposed to call this in the optimizer because all conformances should already be present, type checked, and available from SIL instructions. (This is one area where the interface between the type system an compiler is seriously broken). You should use ConcreteExistentialInfo.lookupConformance() from PR17315 instead.

Indeed, you need one ConcreteExistentialInfo for each argument being replaced

slavapestov · 2018-06-27T20:40:53Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+SILCombiner::propagateConcreteTypeOfInitExistentialsToAllApplyArgs(
+    FullApplySite AI) {
+  /// Check if legal to perform propagation.
+  if (!AI.hasSubstitutions())


Remove this because you check for a polymorphic callee below already

slavapestov · 2018-06-27T20:41:45Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+    Type ContextTy;
+    bool isCopied = false;
+
+    InitExistential = findInitExistential(AI, OrigApplyArg, OpenedArchetype,


Refactor this as discussed

slavapestov · 2018-06-27T20:42:41Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  if (!FnTy->isPolymorphic())
+    return nullptr;
+
+  auto Args = Callee->begin()->getFunctionArguments();


What if the callee does not have a body?

slavapestov · 2018-06-27T20:45:20Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  if (!AI.hasSubstitutions())
+    return nullptr;
+  auto *Callee = AI.getReferencedFunction();
+  if (!Callee || !Callee->shouldOptimize() || Callee->empty())


This restricts the optimization unnecessarily. It should work even if the callee of the apply is not a static function_ref. You should be looking at the operand of the apply's SILFunctionType, and walk the parameters of the function type. The arguments of the callee should not be needed.

slavapestov · 2018-06-27T20:47:40Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -1027,6 +1053,124 @@ SILCombiner::propagateConcreteTypeOfInitExistential(FullApplySite AI) {
  return propagateConcreteTypeOfInitExistential(AI, PD, PropagateIntoOperand);
 }

+/// A peephole optimizer that propagates concrete types of all arguments to
+/// Apply instruction. It handles all the arguments of an apply including self.


Add a comment describing the transformation

slavapestov · 2018-06-27T21:04:17Z

Reminder: check for inout

rajbarik · 2018-08-30T00:24:24Z

@atrick can you review this PR? This PR propagates concrete types to all archetype args, not just self so that generic specializer can create a specialized version. @slavapestov fyi.

slavapestov · 2018-08-31T00:07:45Z

lib/SILOptimizer/SILCombiner/SILCombiner.h

+                     unsigned ArgIdx);
+  SILInstruction *createApplyWithConcreteType(
+      FullApplySite Apply,
+      llvm::SmallDenseMap<unsigned, const ConcreteExistentialInfo *, 8> &CEIs,


Passing SmallDenseMapImpl instead would allow you to avoid hard-coding the size here. Also you should make it 'const'.

Which header file has this SmallDenseMapImpl?

slavapestov · 2018-08-31T00:09:09Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  for (unsigned ArgIdx = 0; ArgIdx < NumApplyArgs; ArgIdx++) {
+    llvm::SmallDenseMap<unsigned, const ConcreteExistentialInfo *, 8>::iterator
+        ArgIt = CEIs.find(ArgIdx);
+    if (ArgIt != CEIs.end()) {


I think the == case should come first, with a 'continue' at the end. Then the rest does not need a whole level of indent.

slavapestov · 2018-08-31T00:09:41Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+              // Do a conformance lookup on this witness requirement using the
+              // existential's conformances. The witness requirement may be a
+              // base type of the existential's requirements.
+              return CEI->lookupExistentialConformance(proto).getValue();


This callback returns an Optional already, I don't think you need to do getValue() here.

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

rajbarik · 2018-09-06T20:54:12Z

@atrick find sometime to review this?

Already handled.

atrick

LGMT other than a couple comments below.

atrick · 2018-09-06T23:48:41Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  // Construct the map for Self to be used for createApplyWithConcreteType.
+  llvm::SmallDenseMap<unsigned, const ConcreteExistentialInfo *> CEIs;
+  CEIs.insert(std::pair<unsigned, const ConcreteExistentialInfo *>(
+      Apply.getNumArguments() - 1, &CEI));


We do this operand index hard-coding all over, but I would prefer Apply.getCalleeArgIndex(Apply.getSelfArgumentOperand()).

atrick · 2018-09-07T00:07:47Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  // of OpenedArchetype in the function signature, but will only rewrite the
+  // Arg operand.
+  //
+  // Note that the language does not allow Arg to occur in contravariant


This comment only applies to Self not Arg. I think we should now say that this bailout check is needed for non-Self arguments, and also needed for Self because even though the language does not allow Self to occur in contravariant position ...

So, I think this bailout is still sufficient. Essentially, for now we only allow specialization on a single generic argument. @slavapestov does it make sense to you?

rajbarik · 2018-09-07T18:35:13Z

Thanks @atrick . I pushed a new commit.

atrick

LGTM

slavapestov · 2018-09-10T06:15:32Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  // references to OpenedArchetype will be substituted. So walk to type to
+  // find all possible references, such as returning Optional<Arg>.
+  if (Apply.getType().getASTType().findIf(
+          [&CEI](Type t) -> bool { return t->isEqual(CEI.OpenedArchetype); })) {


This snippet appears in a few places -- extract a new static function?

slavapestov · 2018-09-10T06:16:24Z

@swift-ci Please test

slavapestov · 2018-09-10T06:16:28Z

@swift-ci Please test source compatibility

swift-ci · 2018-09-10T06:18:12Z

Build failed
Swift Test Linux Platform
Git Sha - 9452e0f271ec44dfed0f0b31e6cd81b1ae742d1c

swift-ci · 2018-09-10T06:18:22Z

Build failed
Swift Test OS X Platform
Git Sha - 9452e0f271ec44dfed0f0b31e6cd81b1ae742d1c

eeckstein · 2018-09-11T19:54:52Z

@rajbarik Unfortunately this broke the asan bot. I reverted it.

rajbarik · 2018-09-11T22:54:03Z

@eeckstein Anything I can do to help?

eeckstein · 2018-09-11T23:17:18Z

That's (most likely) caused by a bug in your code. I recommend to reproduce the issue locally with that asan configuration.

atrick · 2018-09-12T00:34:05Z

@eeckstein @rajbarik normally the log file from the ASAN bot has enough information to give you a clue where to look in the code.

rajbarik · 2018-09-12T17:04:08Z

I see. I never did this before. Could you either point me to the log of ASAN bot or steps to reproduce it at my end?

…

On Tue, Sep 11, 2018 at 5:34 PM Andrew Trick ***@***.***> wrote: @eeckstein <https://github.com/eeckstein> @rajbarik <https://github.com/rajbarik> normally the log file from the ASAN bot has enough information to give you a clue where to look in the code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17370 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwaVCTmSsqLQ_TuqItP8IKC1kNdglRks5uaFaUgaJpZM4Uv6d2> .

rajbarik · 2018-09-12T18:22:07Z

Never mind. I found it at: https://ci.swift.org/job/oss-lldb-asan-osx/11/consoleFull#5770858633122a513-f36a-4c87-8ed7-cbc36a1ec144

…

On Wed, Sep 12, 2018 at 10:04 AM Raj Barik ***@***.***> wrote: I see. I never did this before. Could you either point me to the log of ASAN bot or steps to reproduce it at my end? On Tue, Sep 11, 2018 at 5:34 PM Andrew Trick ***@***.***> wrote: > @eeckstein <https://github.com/eeckstein> @rajbarik > <https://github.com/rajbarik> normally the log file from the ASAN bot has > enough information to give you a clue where to look in the code. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#17370 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AFBwaVCTmSsqLQ_TuqItP8IKC1kNdglRks5uaFaUgaJpZM4Uv6d2 > > . > — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#17370 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwaXiMCGl5oPbi9DrRj8FakmM8_WEmks5uaT64gaJpZM4Uv6d2> .

rajbarik · 2018-09-12T19:32:12Z

@atrick I think I narrowed down the problem. I was storing stack allocated CEI structs in a loop using a map and then was using the map outside the loop later:

for (unsigned ArgIdx = 0; ArgIdx < Apply.getNumArguments(); ArgIdx++) {
 991     auto ArgASTType = Apply.getArgument(ArgIdx)->getType().getASTType();
 992     if (!ArgASTType->hasArchetype())
 993       continue;
 994     const ConcreteExistentialInfo CEI(Apply.getArgumentOperands()[ArgIdx]);
 995     if (!CEI.isValid())
 996       continue;
 997
 998     CEIs.insert(
 999         std::pair<unsigned, const ConcreteExistentialInfo *>(ArgIdx, &CEI));
1000
1001     if (CEI.ConcreteType->isOpenedExistential()) {
1002       // Temporarily record this opened existential def in this local
1003       // BuilderContext before rewriting the witness method.
1004       OpenedArchetypesTracker.addOpenedArchetypeDef(
1005           cast<ArchetypeType>(CEI.ConcreteType), CEI.ConcreteTypeDef);
1006     }

I can think of two ways of fixing this:

make the type of map to be of <unsigned, const ConcreteExistentialInfo> in which case when we store, we copy [this requires that your code

ConcreteExistentialInfo(ConcreteExistentialInfo &) = delete;

be removed and CEIs are copied around. Is there any particular reason you disabled the copy constructor?
2) A ConcreteExistentialInfo be allocated/deallocated on the heap. Lots of boiler plate code be embedded for new/delete. I would prefer the first one though.

Any recommendations?

atrick · 2018-09-12T21:18:15Z

Deleting the copy constructor just signals the original intention that's it's a grouping of local variables that we don't want to accidentally pass-by-copy or store beyond its caller's scope. In general, you can migrate something like this to a collection of multiple elements using unique_ptr to each "Info" object. However, in this case there's nothing inside ConcreteExistentialInfo except pointer-like things, so the default copy constructor is cheap and does the right thing (you can just remove the deleted copy ctor). Copying them into a hash map of dynamically allocated storage one by one is fine.

rajbarik · 2018-09-13T01:10:18Z

@atrick @slavapestov could you review #19287 now?

atrick previously requested changes Jun 20, 2018

View reviewed changes

rajbarik force-pushed the raj-cp-allargs branch from 6103e91 to ba07d61 Compare June 20, 2018 22:16

slavapestov reviewed Jun 27, 2018

View reviewed changes

slavapestov self-assigned this Jun 27, 2018

rajbarik force-pushed the raj-cp-allargs branch 3 times, most recently from 9853879 to 9452e0f Compare August 30, 2018 00:22

rajbarik force-pushed the raj-cp-allargs branch 2 times, most recently from 7ec3892 to 728297c Compare August 30, 2018 18:58

slavapestov reviewed Aug 31, 2018

View reviewed changes

rajbarik force-pushed the raj-cp-allargs branch 2 times, most recently from 32baa7a to d7e59db Compare September 4, 2018 19:41

atrick reviewed Sep 7, 2018

View reviewed changes

Propagate concrete types for all arguments of an apply instruction

0cdcc40

rajbarik force-pushed the raj-cp-allargs branch from d7e59db to 0cdcc40 Compare September 7, 2018 18:33

atrick approved these changes Sep 7, 2018

View reviewed changes

slavapestov reviewed Sep 10, 2018

View reviewed changes

slavapestov approved these changes Sep 10, 2018

View reviewed changes

rajbarik merged commit 7c0bb8c into swiftlang:master Sep 10, 2018

eeckstein mentioned this pull request Sep 11, 2018

Revert "Propagate concrete types of arguments to apply" #19248

Merged

eeckstein mentioned this pull request Sep 11, 2018

[Parse] Tweak a utility function that relies on reading past the end #19230

Merged

rajbarik mentioned this pull request Sep 13, 2018

Propagate concrete types for all arguments of an apply instruction #19287

Merged

Propagate concrete types of arguments to apply #17370

Propagate concrete types of arguments to apply #17370

Uh oh!

Conversation

rajbarik commented Jun 20, 2018

Uh oh!

rajbarik commented Jun 20, 2018

Uh oh!

atrick left a comment • 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!

slavapestov commented Jun 27, 2018

Uh oh!

rajbarik commented Aug 30, 2018

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!

Uh oh!

rajbarik commented Sep 6, 2018

Uh oh!

atrick left a comment • 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!

rajbarik commented Sep 7, 2018

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Sep 10, 2018

Uh oh!

slavapestov commented Sep 10, 2018

Uh oh!

swift-ci commented Sep 10, 2018

Uh oh!

swift-ci commented Sep 10, 2018

Uh oh!

eeckstein commented Sep 11, 2018

Uh oh!

rajbarik commented Sep 11, 2018

Uh oh!

eeckstein commented Sep 11, 2018

Uh oh!

atrick commented Sep 12, 2018

Uh oh!

rajbarik commented Sep 12, 2018 via email

Uh oh!

rajbarik commented Sep 12, 2018 via email

Uh oh!

rajbarik commented Sep 12, 2018

Uh oh!

atrick commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

atrick left a comment •

edited

Loading

atrick left a comment •

edited

Loading

atrick commented Sep 12, 2018 •

edited

Loading