Attempt to rewrite unnecessary constructor applications in the plugin #5649
Conversation
| @@ -823,22 +829,91 @@ compileExpr e = traceCompilation 2 ("Compiling expr:" GHC.<+> GHC.ppr e) $ do | |||
| body' <- compileExpr body | |||
| pure $ PIR.mkLet annMayInline PIR.Rec binds body' | |||
|
|
|||
| GHC.Case scrutinee b t alts -> | |||
| compileCase (const . GHC.isDeadOcc . GHC.occInfo . GHC.idInfo) True binfo scrutinee b t alts | |||
There was a problem hiding this comment.
was rewriteOpaque supposed to become a plugin option?
There was a problem hiding this comment.
No because I don't see a reason why it should be turned off.
There was a problem hiding this comment.
Ah, I couldn't see what else it was for, but I see you're passing it as false the second time.
| , let dataConName = GHC.dataConName dataCon | ||
| , let dataConModule = GHC.moduleNameString . GHC.moduleName <$> GHC.nameModule_maybe dataConName | ||
| , let dataConBase = GHC.occNameString (GHC.nameOccName dataConName) | ||
| , (dataConModule, dataConBase) `elem` opaqueTypes -> do |
There was a problem hiding this comment.
This is a case where we really should be using NameInfo. We're looking for exact types here, so there's no reason why we shouldn't be able to identify the exact Name.
There was a problem hiding this comment.
AFAICT, this transformation is totally safe regardless of what data constructor we use it on. If we see this pattern with a non-opaque data constructor, that's still pointless reboxing.
So... maybe we should just drop the whole "is this an opaque data constructor business". The downside is that then I guess this would fire more frequently.
There was a problem hiding this comment.
Good point. Will try that.
There was a problem hiding this comment.
Done; it actually led to some small performance wins!
| isDead' b = not . any (== b) . universeBi | ||
| -- If some binders are still alive, we have to give up (rather than trying to rewrite | ||
| -- opaque constructor applications again, which will loop), hence `False`. | ||
| compileCase isDead' False binfo scrutinee binder t [GHC.Alt con bs (transform f body)] |
There was a problem hiding this comment.
I don't love the recursion here, since it makes me think about whether or not we can re-enter the same branch. I'm sure it's fine, but I'd be tempted to structure it as a two-phase thing.
There was a problem hiding this comment.
Please elaborate how a two-phase approach looks like.
There was a problem hiding this comment.
I meant, have a version that clearly does do the rewriting and then one that clearly doesn't. But I think you achieve that with the boolean flag, I just hadn't seen that you pass it as False here.
Co-authored-by: Michael Peyton Jones <[email protected]>
Co-authored-by: Michael Peyton Jones <[email protected]>
zliu41
changed the title
|
CI failure is unrelated. |
|
CI failures are spurious |
…IntersectMBO#5649) * Attempt to rewrite opaque constructor applications in the plugin * Update plutus-tx-plugin/src/PlutusTx/Compiler/Expr.hs Co-authored-by: Michael Peyton Jones <[email protected]> * Update plutus-tx-plugin/src/PlutusTx/Compiler/Expr.hs Co-authored-by: Michael Peyton Jones <[email protected]> * Rewrite all unnecessary constructor applications * cabal files --------- Co-authored-by: Michael Peyton Jones <[email protected]>
This should make the "Unsupported feature: Cannot case on a value on type" error happen much less often, if not eliminating it entirely. Tested on 38b6cd9.