Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change bit indexing to use Word parameter in replaceBit# (#1983) #1993

Closed
wants to merge 1 commit into from

Conversation

gergoerdi
Copy link
Contributor

This is more of a proof-of-concept for #1983 than a fully tested fix. With it applied, I can see that it is an effective workaround for YosysHQ/yosys#3051, at least in the simple case. I haven't tried anything more complex yet; a good test case should be https://github.com/gergoerdi/clash-calculator which is where I initially noticed this.

@christiaanb
Copy link
Member

This commit shows that Enum could do with an overhaul to use Word; I wonder whether it’s worth submitting a proposal to the CLC. Or are there enumerable things that can have a negative amount?

@alex-mckenna
Copy link
Contributor

Or are there enumerable things that can have a negative amount?

Unless I'm missing your meaning, Int?

@gergoerdi
Copy link
Contributor Author

Or are there enumerable things that can have a negative amount?

Unless I'm missing your meaning, Int?

But you can still enumerate Ints, e.g. 0, 1, -1, 2, -2, .... Or by a raw coercion, 0, 1, ..., maxBound, minBound, minBound + 1, ..., -1.

@christiaanb
Copy link
Member

No, I do mean that it should be toEnum :: Word -> a and fromEnum :: a -> Word

@leonschoorl
Copy link
Member

While it would be convenient for us if the Enum class would be changed to use Words.
It wouldn't really be an improvement for Haskell in general, toEnum 42 :: Bool is just as wrong as toEnum (-1) :: Bool.
And (-1) :: Word isn't even an compile error, just a warning.
Also there is the added confusion around fromEnum (-1 ::Int) == (maxBound :: Word)

@christiaanb
Copy link
Member

Yeah, I know that toEnum and fromEnum are explicitly conversions from and to Int: it even says so in their documentation and in the Haskell report.

But I always read them more in the spirit of: "Class Enum defines operations on sequentially ordered types.", where toEnum a picks the a'th (0-based) element in the sequence; and fromEnum a gives the (0-based) position of a in the sequence.

@martijnbastiaan
Copy link
Member

I'd say if they'd consider a breaking change like that, they should consider using Integer/Natural.

@gergoerdi
Copy link
Contributor Author

I haven't tried anything more complex yet; a good test case should be https://github.com/gergoerdi/clash-calculator which is where I initially noticed this.

Unfortunately, clash-calculator still doesn't work fully with this patch -- the next hurdle is YosysHQ/yosys#3078 but that's outside the world of Clash (and might turn out to be a bug somewhere else in Symbiflow instead of Yosys anyway).

@gergoerdi
Copy link
Contributor Author

I wonder whether it’s worth submitting a proposal to the CLC. Or are there enumerable things that can have a negative amount?

Isn't that, realistically, just a roundabout way of saying this won't get into Clash?

@christiaanb
Copy link
Member

I wonder whether it’s worth submitting a proposal to the CLC. Or are there enumerable things that can have a negative amount?

Isn't that, realistically, just a roundabout way of saying this won't get into Clash?

No, we’ll merge this

@martijnbastiaan
Copy link
Member

@gergoerdi Is there anything that still needs to happen for this PR? I'd like to get this in the 1.6 release (02-02-2022) if possible :)

@DigitalBrains1
Copy link
Member

DigitalBrains1 commented Jan 24, 2022

Some musing.

While discussing my GHC issue report for fromEnum :: Natural, a GHC developer expressed this viewpoint:

IMHO Enum should not been trying to do more then desugar [x .. y], [x..] and [x,y...z] notations. fromEnum and toEnum are handy helpers to specify the deriving, but IMO they didn't need to belong to the class.

This is one developer's opinion, and I disagree on the "only desugaring" when I observe that Enum also has succ and pred and those are pretty good IMHO (although in Clash I much prefer satSucc). But it is the case that the minimal complete definition of Enum is providing fromEnum and toEnum, and the thought that these purely exist to facilitate writing (or deriving) Enum instances does resonate with me. I wonder whether fromEnum and toEnum were ever even meant to be used on an instance rather than only defined for quick and easy instance building (after which you use the other functions).

The takeaway is this: maybe we are using Enum in a manner different from its intent in functions such as:

asyncRom
:: (KnownNat n, Enum addr)
=> Vec n a
-- ^ ROM content, also determines the size, @n@, of the ROM
--
-- __NB:__ must be a constant
-> addr
-- ^ Read address @rd@
-> a
-- ^ The value of the ROM at address @rd@
asyncRom = \content rd -> asyncRom# content (fromEnum rd)
{-# INLINE asyncRom #-}

@martijnbastiaan
Copy link
Member

@alex-mckenna Could you pretty please have a look whether we can include this for 1.6? :)

@alex-mckenna alex-mckenna marked this pull request as ready for review January 31, 2022 08:31
@alex-mckenna
Copy link
Contributor

@kloonbot run_ci 9e64b68

Let's see what kloonbot says

Copy link
Contributor

@alex-mckenna alex-mckenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the whole, I think this is fine. I would like to see a regression test if possible, although I think that means adding Yosys as a vendor tool in the testsuite. If I make a PR for that, can you make a test for this @gergoerdi?

@gergoerdi
Copy link
Contributor Author

I'm not sure how I feel about this change, now that YosysHQ/yosys#3051 has a candidate fix. If using signed types to index bits is valid Verilog/VHDL, and the practical consideration of working around a Yosys bug no longer applies, then is there an advantage (other than maybe decreasing surprises) to this change?

@alex-mckenna
Copy link
Contributor

Hmm yes, if Yosys have a fix then I agree we don't think we really need this since it is allowed in Verilog / VHDL. I don't think there's much to be said for less surprise given how many Haskell APIs index with Int when Word is arguably more correct anyway...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants