SimplifyCFG: General jump threading needs to update all available values when updating SSA

[aschwaighofer]

This was introduced by refactors in this area.

rdar://61232730

[aschwaighofer]

@swift-ci Please test

[atrick]

Ok, this should work but... somehow we need to make sure canCloneInstruction is called for every instruction in the block, otherwise we have a different correctness problem. Checking for 'needsSSAUpdate' currently ensures that it will be called.

I'm guessing the problem is that SimplifyCFG materializes an enum after calling canCloneBlock. The better fix would be to have SimplifyCFG call canCloneInstruction on the new instruction, rather than add a flag to the Cloner interface.

We could rename canCloneInstruction to checkCloneInstruction to indicate that it has side effects.

The code you added to updateSSAAfterCloning could remain under an assert

if (!needsUpdate) {
  for (auto &inst : *origBB) {
      for (auto res : inst.getResults()) {
        assert(!isUsedOutsideOfBlock(res))
        }
      }
    }
}

But would that be any better than the assert that you currently hit with this bug?

[aschwaighofer]

Hmm, the problem was the existing:

  %17 = tuple_element_addr %15 : $*(FakeOptional<Int>, FakeOptional<S>), 1

It would seem then the SinkAddressProjections stuff does not work then.

[aschwaighofer]

I'll take another look at what is going on there.

[atrick]

I'm happy to debug this immediately after my current debugging session.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e1c013392d0151ed825ab42339286957d559f352

[aschwaighofer]

Got it. When we walk back an address projection and we meet an instruction that produces an address but is not pure we need to set needsSSAUpdate instead of doing nothing.

diff --git a/lib/SILOptimizer/Utils/BasicBlockOptUtils.cpp b/lib/SILOptimizer/Utils/BasicBlockOptUtils.cpp
index 63baa6934a7..a1389ad299e 100644
--- a/lib/SILOptimizer/Utils/BasicBlockOptUtils.cpp
+++ b/lib/SILOptimizer/Utils/BasicBlockOptUtils.cpp
@@ -196,6 +196,8 @@ bool SinkAddressProjections::analyzeAddressProjections(SILInstruction *inst) {
       if (addressProj->isPure()) {
         projections.push_back(addressProj);
         return true;
+      } else {
+        inBlockDefs.insert(def);
       }
     }
     // Can't handle a multi-value or unclonable address producer.

[aschwaighofer]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - e1c013392d0151ed825ab42339286957d559f352

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e1c013392d0151ed825ab42339286957d559f352

[atrick]

This is supposed to maintain an invariant that inBlockDefs never contains an address-type value. Otherwise, cloning the block is illegal because address-type phis are illegal.
So, the reason we have a loop here:

for (SILValue operandVal : projections[idx]->getOperandValues())
      pushOperandVal(operandVal);

Instead of just checking one operand, is to make sure all of the operands are added to inBlockDefs. But if any of those have an address type and cannot be cloned, it needs to bail. It looks to me like there's just an obvious missing error check here:

for (SILValue operandVal : projections[idx]->getOperandValues())
  if (!pushOperandVal(operandVal))
    return false;
}

It looks like I intended to write that but somehow it got lost.

Unfortunately that means this example will not get jump-threaded. If that's a problem, it's probably a separate bug to deal with!

[aschwaighofer]

@swift-ci Please test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 7549bfed06e47f59a60a1f7327fb3301a5f45eb6

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 7549bfed06e47f59a60a1f7327fb3301a5f45eb6