Prefix names of inlined functions #958

alex-mckenna · 2019-12-06T10:44:43Z

Identifiers which are assigned an inlined function now have a
prefix with the name of the function that was inlined. This makes
it clearer what the provinence of an assigment is in HDL.

christiaanb · 2019-12-06T11:17:19Z

diff --git a/clash-lib/src/Clash/Normalize/Transformations.hs b/clash-lib/src/Clash/Normalize/Transformations.hs
index 84cfcbe7..cadb0cf1 100644
--- a/clash-lib/src/Clash/Normalize/Transformations.hs
+++ b/clash-lib/src/Clash/Normalize/Transformations.hs
@@ -1722,9 +1722,9 @@ recToLetRec (TransformContext is0 []) e = do
       = and (zipWith (eqArg tcm) args args2)
     eqApp _ _ _ _ = False

-    eqArg _ v1 v2@(Var {})
+    eqArg _ v1 v2@(stripTicks -> Var {})
       = v1 == v2
-    eqArg tcm v1 v2@(collectArgs -> (Data _, args'))
+    eqArg tcm v1 v2@(collectArgs . stripTicks -> (Data _, args'))
       | let t1 = termType tcm v1
       , let t2 = termType tcm v2
       , t1 == t2                                                                                                           , t1 == t2

Fixes Feedback/Fib.hs

The gist of the above fix is that the recToLetRec transformation needs to check whether a self-recursive function is applied to expressions that are "sementically" equal to variable references to its arguments; i.e. whether it passes along its arguments unchanged. Ticks don't change the operational semantics, so that's why we strip them. You probably want to look at recToLetRec some more and see if we need to strip even more ticks.

alex-mckenna · 2019-12-06T11:27:05Z

Your ability to conjure fixes out of thin air never ceases to amaze 👍

christiaanb · 2019-12-06T11:30:20Z

The name conflicts are a result in not being in normal form, I'm seeing:

LambdaDrop.core9185 after normalization:

λ(x1 :: Clash.Signal.Internal.Signal8214565720324203345
          "System"
          (GHC.Maybe.Maybe3674937295934324792
             Clash.Sized.Internal.BitVector.Bit8214565720324203376)) ->
letrec
  result13254 :: Clash.Sized.Internal.BitVector.Bit8214565720324203376
  = <prefixName>"core1_"
  letrec
    x6989586621679501791 :: Clash.Sized.Internal.BitVector.Bit8214565720324203376
    = case x1[LocalId] of
        GHC.Maybe.Just3891110078048108571
          (c$sel13252 :: Clash.Sized.Internal.BitVector.Bit8214565720324203376) ->
          c$sel13252[LocalId]
  in case x1[LocalId] of
       GHC.Maybe.Nothing3891110078048108568  ->
         Clash.Sized.Internal.BitVector.low
       GHC.Maybe.Just3891110078048108571
         (x6989586621679501795 :: Clash.Sized.Internal.BitVector.Bit8214565720324203376) ->
         x6989586621679501791[LocalId]
in result13254[LocalId]

i.e. a nested-letbinding. Haven't figured out why flattenLet is failing though.

christiaanb · 2019-12-06T11:46:47Z

Okay, it's not flattenLet that's failing to fire; it's topLet firing too early: it needs to fire after all lambda's and ticks:

diff --git a/clash-lib/src/Clash/Normalize/Transformations.hs b/clash-lib/src/Clash/Normalize/Transformations.hs
index 84cfcbe7..860ea32b 100644
--- a/clash-lib/src/Clash/Normalize/Transformations.hs
+++ b/clash-lib/src/Clash/Normalize/Transformations.hs
@@ -715,7 +715,7 @@ nonRepANF _ e = return e
 -- the body is a variable-reference.
 topLet :: HasCallStack => NormRewrite
 topLet (TransformContext is0 ctx) e
-  | all (\c -> isLambdaBodyCtx c || isTickCtx c) ctx && not (isLet e)
+  | all (\c -> isLambdaBodyCtx c || isTickCtx c) ctx && not (isLet e) && not (isTick e)
   = do
   untranslatable <- isUntranslatable False e
   if untranslatable
@@ -723,6 +723,9 @@ topLet (TransformContext is0 ctx) e
     else do tcm <- Lens.view tcCache
             argId <- mkTmBinderFor is0 tcm (mkUnsafeSystemName "result" 0) e
             changed (Letrec [(argId, e)] (Var argId))
+ where
+  isTick Tick {} = True
+  isTick _ = False

 topLet (TransformContext is0 ctx) e@(Letrec binds body)
   | all (\c -> isLambdaBodyCtx c || isTickCtx c) ctx

christiaanb · 2019-12-06T11:53:28Z

And finally, splitNormalized should take ticked Letrecs into account:

diff --git a/clash-lib/src/Clash/Netlist/Util.hs b/clash-lib/src/Clash/Netlist/Util.hs
index 7cadafed..a727bfbd 100644
--- a/clash-lib/src/Clash/Netlist/Util.hs
+++ b/clash-lib/src/Clash/Netlist/Util.hs
@@ -70,13 +70,13 @@ import           Clash.Core.Subst
    extendInScopeIdList, mkSubst, substTm)
 import           Clash.Core.Term
   (Alt, LetBinding, Pat (..), Term (..), TickInfo (..), NameMod (..),
-   collectArgsTicks)
+   collectArgsTicks, collectTicks)
 import           Clash.Core.TyCon
   (TyConName, TyConMap, tyConDataCons)
 import           Clash.Core.Type         (Type (..), TypeView (..),
                                           coreView1, splitTyConAppM, tyView, TyVar)
 import           Clash.Core.Util
-  (collectBndrs, stripTicks, substArgTys, termType, tyLitShow)
+  (collectBndrs, stripTicks, substArgTys, termType, tyLitShow, mkTicks)
 import           Clash.Core.Var
   (Id, Var (..), mkLocalId, modifyVarName, Attr')
 import           Clash.Core.VarEnv
@@ -129,9 +129,9 @@ splitNormalized
   -> Term
   -> (Either String ([Id],[LetBinding],Id))
 splitNormalized tcm expr = case collectBndrs expr of
-  (args,Letrec xes e)
+  (args,collectTicks -> (Letrec xes e,ticks))
     | (tmArgs,[]) <- partitionEithers args -> case stripTicks e of
-        Var v -> Right (tmArgs,xes,v)
+        Var v -> Right (tmArgs,map (second (`mkTicks` ticks)) xes,v)
         _     -> Left ($(curLoc) ++ "Not in normal form: res not simple var")
     | otherwise -> Left ($(curLoc) ++ "Not in normal form: tyArgs")
   _ ->

martijnbastiaan · 2019-12-11T08:29:13Z

clash-lib/src/Clash/Netlist/Util.hs

@@ -129,9 +129,9 @@ splitNormalized
  -> Term
  -> (Either String ([Id],[LetBinding],Id))
 splitNormalized tcm expr = case collectBndrs expr of
-  (args,Letrec xes e)
+  (args, collectTicks -> (Letrec xes e, ticks))


My understanding is: ticks can wrap any term, so any time you match on a term you should make sure that

it either has no ticks

or to call collectTicks

This seems like an impossible thing to do correctly, so ideally I'd like to see a solution where the compiler forces us to handle ticks. Could we think about this please? (Not for this PR though.)

martijnbastiaan

Core logic looks good. Please remove debugging statements.

martijnbastiaan · 2019-12-11T08:29:18Z

clash-lib/src/Clash/Normalize.hs

@@ -19,6 +19,9 @@

 module Clash.Normalize where

+import Debug.Trace -- TODO


I think we should have more random TODOs scattered in the code. After all, our work is never done 😉

martijnbastiaan · 2019-12-11T08:29:40Z

clash-lib/src/Clash/Normalize.hs

@@ -323,6 +331,7 @@ flattenCallTree (CBranch (nm,(nm',sp,inl,tm)) used) = do
    _  -> do
      -- To have a cheap `appProp` transformation we need to
      -- deshadow, see also Note [AppProp no-shadow invariant]
+      -- TODO Tick inline?


What do you mean with "tick inline"?

martijnbastiaan · 2019-12-11T08:31:31Z

clash-lib/src/Clash/Normalize/Transformations.hs

+                      , "\nRun with '-fclash-inline-limit=N' to increase"
+                      , " the inlining limit to N."
+                      ])
+              (return e)


We should spend some time on building a quasiquoter.. (Again, not for this PR.)

martijnbastiaan · 2019-12-11T08:31:49Z

clash-lib/src/Clash/Normalize/Transformations.hs

@@ -399,9 +401,16 @@ inlineNonRep (TransformContext localScope _) e@(Case scrut altsTy alts)
        case (nonRepScrut, bodyMaybe) of
          (True,Just (_,_,_,scrutBody0)) -> do
            Monad.when noException (zoomExtra (addNewInline f cf))
+
+            traceM  $ "inlineNonRep: " <> (Text.unpack . nameOcc $ varName f)


martijnbastiaan · 2019-12-11T08:32:08Z

clash-lib/src/Clash/Normalize/Transformations.hs

@@ -968,8 +980,13 @@ inlineWorkFree (TransformContext localScope _) e@(collectArgsTicks -> (Var f,arg
            if isRecBndr
               then return e
               else do
-                 -- See Note [AppProp no-shadow invariant]
-                 changed (mkApps (mkTicks (deShadowTerm localScope body) ticks) args)
+                 traceM  $ "inlineWorkFree: " <> (Text.unpack . nameOcc $ varName f)


martijnbastiaan · 2019-12-11T08:32:20Z

clash-lib/src/Clash/Normalize/Transformations.hs

+               -- TODO This currently doesn't change clashing names when a
+               -- tick is added, causing tests like LambdaDrop to fail.
+
+               traceM  $ "inlineSmall: " <> (Text.unpack . nameOcc $ varName f)


martijnbastiaan · 2019-12-11T08:33:46Z

clash-lib/src/Clash/Normalize/Util.hs

+-- | A tick to prefix an inlined expression with it's original name.
+--


An example would be nice here

martijnbastiaan · 2019-12-11T08:34:07Z

clash-lib/src/Clash/Core/Evaluator.hs

@@ -356,11 +357,18 @@ force :: Heap -> Stack -> Id -> Maybe State
 force (Heap gh g@(GPureHeap gbl) h ids is) k x' = case lookupVarEnv x' h of
    Nothing -> case lookupVarEnv x' gbl of
      Just e | isGlobalId x'
-        -> Just (Heap gh (GPureHeap (delVarEnv gbl x')) h ids is,GUpdate x':k,e)
+        -> let e' = tickExpr e
+            in do traceM $ "force: " <> (Text.unpack . nameOcc $ varName x')


All of these traces will be removed for the final PR I assume?

Ah right, I thought this was the final PR.

Identifiers which are assigned an inlined function now have a prefix with the name of the function that was inlined. This makes it clearer what the provinence of an assigment is in HDL.

alex-mckenna requested review from christiaanb and martijnbastiaan December 6, 2019 10:44

alex-mckenna force-pushed the name-prefixes-inline-fns branch from b079ff1 to 197eefb Compare December 9, 2019 09:24

alex-mckenna marked this pull request as ready for review December 9, 2019 09:24

martijnbastiaan reviewed Dec 11, 2019

View reviewed changes

martijnbastiaan approved these changes Dec 11, 2019

View reviewed changes

alex-mckenna force-pushed the name-prefixes-inline-fns branch from 197eefb to 973c794 Compare December 17, 2019 08:43

christiaanb approved these changes Dec 17, 2019

View reviewed changes

Prefix names of inlined functions

ac1dd7c

Identifiers which are assigned an inlined function now have a prefix with the name of the function that was inlined. This makes it clearer what the provinence of an assigment is in HDL.

alex-mckenna force-pushed the name-prefixes-inline-fns branch from d96bb55 to ac1dd7c Compare December 17, 2019 12:46

martijnbastiaan merged commit 408f76b into master Dec 17, 2019

martijnbastiaan deleted the name-prefixes-inline-fns branch December 17, 2019 14:16

christiaanb mentioned this pull request Dec 19, 2019

Add name prefixes when functions are inlined #952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefix names of inlined functions #958

Prefix names of inlined functions #958

alex-mckenna commented Dec 6, 2019

christiaanb commented Dec 6, 2019 •

edited

Loading

alex-mckenna commented Dec 6, 2019

christiaanb commented Dec 6, 2019

christiaanb commented Dec 6, 2019

christiaanb commented Dec 6, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan left a comment

martijnbastiaan Dec 11, 2019

alex-mckenna Dec 12, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

martijnbastiaan Dec 11, 2019

christiaanb Dec 11, 2019

martijnbastiaan Dec 11, 2019

		@@ -19,6 +19,9 @@

		module Clash.Normalize where

		import Debug.Trace -- TODO

		-- \| A tick to prefix an inlined expression with it's original name.
		--

Prefix names of inlined functions #958

Prefix names of inlined functions #958

Conversation

alex-mckenna commented Dec 6, 2019

christiaanb commented Dec 6, 2019 • edited Loading

alex-mckenna commented Dec 6, 2019

christiaanb commented Dec 6, 2019

christiaanb commented Dec 6, 2019

christiaanb commented Dec 6, 2019

Choose a reason for hiding this comment

martijnbastiaan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christiaanb commented Dec 6, 2019 •

edited

Loading