Improved `-fclash-debug-*` flags by alex-mckenna · Pull Request #1847 · clash-lang/clash-compiler

alex-mckenna · 2021-06-08T14:26:48Z

The old DebugLevel style debug options are replaced with a more granular style, where different options can be set independently. This allows things not previously possible, such as counting transformations without having to print any normalized core.

The old -fclash-debug flag still works in the same way for backwards compatibility. Additional command line flags are now available to directly set options from the new type:

-fclash-debug-invariants checks for invariants and displays warnings / errors. Setting this alone is equivalent to DebugSilent.
-fclash-debug-info sets the amount of information to display when applying transformations. This overlaps a lot with the old DebugLevel type, but is named more consistently.
-fclash-debug-count-transformations counts the number of transformations applied (without showing any normalized core like DebugCount did)

Additionally, the debugging options are now in their own type (DebugOpts) which is accessed directly through the RewriteEnv in apply / applyDebug instead of having the individual debugging options passed as inputs to the function. This simplifies the logic slightly in these functions and avoids needing to call applyDebug recursively with modified values.

Fixes #1800

Still TODO:

Write a changelog entry (see changelog/README.md)
Check copyright notices are up to date in edited files

martijnbastiaan · 2021-06-08T14:36:30Z

+instance Hashable DebugOpts where
+  hashWithSalt s DebugOpts{..} =
+    s `hashWithSalt`
+    opt_invariants `hashWithSalt`
+    opt_transformationInfo `hashSet`
+    opt_transformations `hashWithSalt`
+    opt_countTransformations `hashWithSalt`
+    opt_transformationsFrom `hashWithSalt`
+    opt_transformationsLimit `hashWithSalt`
+    opt_historyFile
+   where
+    hashSet = Set.foldl' hashWithSalt
+    infixl 0 `hashSet`


Any reason to not just derive Hashable?

I did wonder about that. Hashable isn't derived for ClashOpts, so when I defined this one I just moved the relevant lines from that instance + added the ones for the new fields to be safe

Hmmm, my guess is that that's a leftover from a feature/hack we've since moved to the right place?:

clash-compiler/clash-lib/src/Clash/Driver/Manifest.hs

Lines 370 to 408 in aaf1559

optsHash = hash opts {

-- Ignore the following settings, they don't affect the generated HDL:

-- 1. Debug

opt_dbgLevel = DebugNone

, opt_dbgTransformations = Set.empty

, opt_dbgRewriteHistoryFile = Nothing

-- 2. Caching

, opt_cachehdl = True

-- 3. Warnings

, opt_primWarn = True

, opt_color = Auto

, opt_errorExtra = False

, opt_checkIDir = True

-- 4. Optional output

, opt_edalize = False

-- Ignore the following settings, they don't affect the generated HDL. However,

-- they do influence whether HDL can be generated at all.

--

-- We therefore check whether the new flags changed in such a way that

-- they could affect successful compilation, and use that information

-- to decide whether to use caching or not (see: XXXX).

--

-- 1. termination measures

, opt_inlineLimit = 20

, opt_specLimit = 20

-- 2. Float support

, opt_floatSupport = False

-- Finally, also ignore the HDL dir setting, because when a user moves the

-- entire dir with generated HDL, they probably still want to use that as

-- a cache

, opt_hdlDir = Nothing

}

I think we can just remove it and derive Hashable - in a separate commit of course so we can rollback if necessary.

Ah, Set String has no Hashable instance, so deriving would fail

And same for OverridingBool in ClashOpts. So I guess without making any more *.Extra modules we wouldn't be able to derive these instances

Ah. Do we rely on the ordering? HashSet is generally preferable anyway. Which would leave OverridingBool - I guess an orphan instance wouldn't hurt.

Do we rely on the ordering?

No, it's just a set of transformations which are whitelisted for showing in the debug logs. I had thought that there should probably be a later commit to change it from String to Text, maybe that should be a commit to change it from Set String to HashSet Text?

I think I'm going to save this kind of type shake-up for a future PR, there are a few places where the types in rewrite / normalize could be improved and fiddling with types seems a bit tangential to this PR

alex-mckenna · 2021-06-11T12:08:40Z

@leonschoorl I've changed applyDebug so it reports unexpected changes when dbg_invariants is on, as opposed to when the new analogue of DebugApplied is on. So if something like #1848 happens again the testsuite will pick it up

Things like the count of transformations applied and the offset / limits for transformations when debugging cannot be negative, so it makes more sense to use Word instead of Int for these.

leonschoorl · 2021-06-15T14:34:21Z

Wow, the term in IntegralTB with 8.10 is so big is doesn't even get to print the actually error.

alex-mckenna · 2021-06-15T14:43:58Z

That's how you know it's ~~working~~ doing something. I guess I need to actually deal with the type change error instead of just downgrading it to a trace, it feels wrong to not include it in opt_invariants, or make it depend on an unrelated debugging option

christiaanb · 2021-06-15T18:53:09Z

Don't change when type-change-errors are reported compared to how it is now on HEAD of master. The reporting is very fragile (asserts often) because we throw away all the casts. Until we properly support casts I wouldn't worry too much about these type-change-errors.

alex-mckenna · 2021-06-15T19:29:21Z

Don't change when type-change-errors are reported compared to how it is now on HEAD of master.

Yeah, seems reasonable. It'll be a little non-obvious to read with the new check (what does the amount of info printed about transformed terms have to do with type errors?) but the behaviour will be identical. I'll leave a comment for archaeology / to reference to #1064 so it's clearer to anyone who happens across it in the future

The debugging options for the compiler are often used together, yet are passed individually to the rewrite system. Keeping debugging options together helps expose less unnecessary detail.

leonschoorl · 2021-06-17T10:18:30Z

+      -- not keep casts, so "illegally" changing between `Signal dom a` and `a`
+      -- will trigger this error for many designs.
+      --
+      -- This should be changed when #1064 (PR to keep casts in core) is merged.


Apart from casts, wasn't this also triggered by separateArguments?

The implicit params thing? Yeah, I didn't include that because by the time the signal cast PR is in it might not be an issue / there might be other small issues so I didn't want to get too into detail past the most immediate obstacle

alex-mckenna requested a review from leonschoorl June 8, 2021 14:26

martijnbastiaan reviewed Jun 8, 2021

View reviewed changes

alex-mckenna force-pushed the issue-1800 branch 2 times, most recently from 3e89582 to 1237446 Compare June 8, 2021 15:20

leonschoorl reviewed Jun 10, 2021

View reviewed changes

Comment thread clash-lib/src/Clash/Driver/Types.hs

alex-mckenna force-pushed the issue-1800 branch 2 times, most recently from 2892b80 to e4520b3 Compare June 11, 2021 12:06

alex-mckenna force-pushed the issue-1800 branch from e4520b3 to 03b0156 Compare June 11, 2021 12:47

leonschoorl mentioned this pull request Jun 14, 2021

Report changes in simpleCSE #1848

Merged

3 tasks

Use Word instead of Int in transformation counts

4d2c6a0

Things like the count of transformations applied and the offset / limits for transformations when debugging cannot be negative, so it makes more sense to use Word instead of Int for these.

alex-mckenna force-pushed the issue-1800 branch 2 times, most recently from 7240791 to 5a93512 Compare June 15, 2021 12:42

Move debug options to new DebugOpts type

8042580

The debugging options for the compiler are often used together, yet are passed individually to the rewrite system. Keeping debugging options together helps expose less unnecessary detail.

alex-mckenna force-pushed the issue-1800 branch from 5a93512 to 8042580 Compare June 16, 2021 07:22

alex-mckenna requested a review from leonschoorl June 16, 2021 10:11

leonschoorl approved these changes Jun 17, 2021

View reviewed changes

alex-mckenna merged commit ad51aea into master Jun 18, 2021

alex-mckenna deleted the issue-1800 branch June 18, 2021 09:13

	optsHash = hash opts {
	-- Ignore the following settings, they don't affect the generated HDL:

	-- 1. Debug
	opt_dbgLevel = DebugNone
	, opt_dbgTransformations = Set.empty
	, opt_dbgRewriteHistoryFile = Nothing

	-- 2. Caching
	, opt_cachehdl = True

	-- 3. Warnings
	, opt_primWarn = True
	, opt_color = Auto
	, opt_errorExtra = False
	, opt_checkIDir = True

	-- 4. Optional output
	, opt_edalize = False

	-- Ignore the following settings, they don't affect the generated HDL. However,
	-- they do influence whether HDL can be generated at all.
	--
	-- We therefore check whether the new flags changed in such a way that
	-- they could affect successful compilation, and use that information
	-- to decide whether to use caching or not (see: XXXX).
	--
	-- 1. termination measures
	, opt_inlineLimit = 20
	, opt_specLimit = 20

	-- 2. Float support
	, opt_floatSupport = False

	-- Finally, also ignore the HDL dir setting, because when a user moves the
	-- entire dir with generated HDL, they probably still want to use that as
	-- a cache
	, opt_hdlDir = Nothing
	}

Conversation

alex-mckenna commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Still TODO:

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alex-mckenna commented Jun 11, 2021

Uh oh!

leonschoorl commented Jun 15, 2021

Uh oh!

alex-mckenna commented Jun 15, 2021

Uh oh!

christiaanb commented Jun 15, 2021

Uh oh!

alex-mckenna commented Jun 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alex-mckenna commented Jun 8, 2021 •

edited

Loading