bsc: remove use of old-time
#721
Conversation
A long time coming (no pun.) This removes all uses of `old-time` from the compiler and replaces it with equivalents from `time`; eliminating a 3rd party dependency needed on top of GHC. Most of the translations are mechanical and rather straight forward. At the same time, I also: - Removed some extra use of `TimeInfo` in `SimBlocksToC` - Removed all references to `ClockTime` (2 extra lines of code in `GenABin`) Signed-off-by: Austin Seipp <[email protected]>
Not in use by the compiler. Just a bit of dead code. Signed-off-by: Austin Seipp <[email protected]>
thoughtpolice
force-pushed
the
aseipp/push-lvrvtzvuyqxy
branch
from
9dde13b
to
e646949
Compare
There was a problem hiding this comment.
At the moment, BSC compiles with all versions of GHC back to 7.10.3 (which GHCUP still offers). This PR will presumably restrict us to newer GHC versions -- but can you saw what that earliest GHC version is? I attempted to build with 7.10.3 and got an error about nominalDiffTimeToSeconds not in scope, but actually I'm surprised that it didn't say that Data.Time doesn't exist, since that's not listed in the base -- so where is it coming from, and at what version? Anyway, nominalDiffTimeToSeconds is available since 1.9.1 of Data.Time -- but where do I find what version is available to GHC versions, if not from the base version?
I haven't looked deeply at what you're doing, but have given some comments. I see that some tests fail, for BSC and BDW. Do you know what those are about?
| @@ -49,7 +49,6 @@ data ABinModInfo = | |||
| -- the name of the BSV package which defined this module | |||
| abmi_src_name :: String, | |||
| -- time when BSC was called to compile the .ba | |||
There was a problem hiding this comment.
This accompanying comment would need to be removed too
| @@ -92,7 +92,7 @@ $(warning Building unoptimized to work around a bug in GHC 9.2.1) | |||
| GHCOPTLEVEL ?= -O0 | |||
| endif | |||
|
|
|||
| GHC += -Wtabs -fmax-pmcheck-models=800 | |||
| GHC += -Wtabs -fmax-pmcheck-models=800 -freverse-errors | |||
There was a problem hiding this comment.
Does this GHC flag change belong in this PR?
| time_str = calendarTimeToString (toUTCTime clock_time) | ||
|
|
||
| ( time_str, time_secs ) = case mcreation_time of | ||
| Nothing -> ( "1970-01-01 00:00:00 UTC", 0 ) |
There was a problem hiding this comment.
Would it be better to create a UTCTime value representing this and call show on it?
| @@ -489,12 +489,15 @@ convertSchedules flags creation_time top_id def_clk def_rst sb_map ff_map | |||
| get_creation_time = function (userType "time_t") | |||
| (mkScopedVar "get_creation_time") | |||
| [] | |||
| (TimeInfo _ clock_time@(TOD t _)) = if (timeStamps flags) | |||
There was a problem hiding this comment.
Why did you remove the flag check from here and put it in the caller?
| @@ -1339,10 +1339,14 @@ genModuleC errh flags dumpnames time0 toplevel abis = | |||
| start flags DFsimBlocksToC | |||
| let sb_map = M.fromList $ map (\sb -> (sb_id sb,sb)) | |||
| (simblocks_opt ++ primBlocks) | |||
| creation_time = time | |||
There was a problem hiding this comment.
This adds the flag check that was removed from simBlocksToC. But you also added a call to getCurrentTime -- what was your thinking for that?
| time_flags = if (timeStamps flags) | ||
| then [ "--creation_time", show t] | ||
| else [] | ||
| time_flags = maybe [] (\t -> ["--creation_time", show t]) mcreation_time |
There was a problem hiding this comment.
Ah, this explains why you moved the flag check: you unified two flag checks into one, which has to be done in the caller, and the Maybe result is passed to both places.
This use of maybe is less readable than the if-then-else, in my opinion.
| @@ -1806,10 +1810,8 @@ cxxLink errh flags toplevel names creation_time = do | |||
| unless (quiet flags) $ putStrLnF ("Simulation shared library created: " ++ soFile) | |||
| -- Write a script to execute bluesim.tcl with the .so file argument | |||
| let bluesim_cmd = "$BLUESPECDIR/tcllib/bluespec/bluesim.tcl" | |||
| (TimeInfo _ (TOD t _)) = creation_time | |||
There was a problem hiding this comment.
This is the source of the testsuite failures, I believe. Previously, the value was time in seconds, so the generated Bluetcl script (that invokes Bluesim) would have these arguments on the exec command line:
--creation_time 1725535443
But now, I'm seeing this:
--creation_time 2024-09-05 11:08:22.302155 UTC
Which has spaces and would need to be in quotes to be treated as a single argument. Without quotes, executing the script gives this:
Error: invalid option '11:08:22.302155'
But adding quotes doesn't help, because the flag is expected the creation time in seconds, so you'll need to use the time in seconds here. Instead of having SImBlocksToC compute the time in seconds, I'd guess you'd want to lift that out too, and have it computed here, and just pass in a pair, of the string and the time in seconds.
FYI, you can see the boot libraries shipped with each GHC version since 7.0.1 here: https://gitlab.haskell.org/ghc/ghc/-/wikis/commentary/libraries/version-history Going to the |
A long time coming (no pun.) This removes all uses of
old-timefrom the compiler and replaces it with equivalents fromtime; eliminating a 3rd party dependency needed on top of GHC. Most of the translations are mechanical and rather straight forward.At the same time, I also:
TimeInfoinSimBlocksToCClockTime(2 extra lines of code inGenABin)