Internal error after normalization when synthesising vhdl in 1.6.2 #2149
Comments
|
I think this is about what's happening: topEntity :: Word -> Signed 32
topEntity = fromIntegralWhich results in basically the same error: |
|
None of the errors occur in 1.5.0pre. I'll check that your example above causes no error there. Yes, that is so:
But it doesn't look really likely to be relevant to me as everything in sight is a stream to stream function, including topEntity. {-# ANN kpu_test32 One thing I do notice is that the top level is a foo = bar ... where bar :: ....; bar = ... . I think there's a reason for that but I forget what. Something to do with specialization. It looks like a silly thing to do, but it is done, so presumably there is a reason. Thanks for the suggestion. PTB I could try locating it by module, but it'll be a lot of work. |
|
I would suggest varying the patch to not produce error but instead just carry on leaving some mark that I can look for in the eventual output, and hence locate where the cause of this is, hopefully. |
@leonschoorl Does this happen pre 1.5 too? If it doesn't, that's a pretty good indication this is @pbreuer's issue. |
|
The only earlier version I have compiled up is 1.1.0 on a 32-bit machine. the 1.5.0pre I have is built July 7 2021. I can locate it more exactly if you point at a definitive changelog to look at. There is a CHANGELOG.md.save0 but that goes up to 1.4.2 only (May 18 2021). I really don't recognize anything like that Word->Signed function at top level. It's all stream to stream. Of course there must be tons like that at a lower level, but I would expect it to be normalized out (my impression is that normalization is failing). Perhaps I should look for inadvertent NOINLINEs that magically were disregarded earlier ... I can also try bisecting between versions to locate the problem. Meanwhile I'm begging for a patch that carries on regardless where before it errored, but leaves some recognizable mark in the final output. Any suggestions? On a strategic level, the vhdl produced by 1.5.0pre runs fine under simulation in ghdl, for thousands of cycles requiring about 100GB of RAM so there appears to be nothing wrong with what 1.5.0pre does. Can you perhaps give me an idea of the abstract problem hinted at here? I may get an idea from that. Should 1.6.2 synthesise that Word->Signed function or shouldn't it? It looks synthesizable to me as it just the identity on 32 1-bit inputs! That is 32 straight through wires, or one straight-through 32-bit wide cable. Do you synthesize for straight stateless logic functions? I'm sorry - I don't know because everything I have written has clocks and state in. Otherwise .. please send patch that leaves a mark in the vhdl output but carries on. I don't care what it does after that point. It could die, as far as I care for this purpose, so long as there is output to look at. Regards PTB PS. I have just tried synthesizing that Word->Signed function under 1.1.0 and it works fine:
So the problem with that (if it is a problem) appears to be in 1.6.2. |
Thanks, that's very helpful! That's a pretty strong indicator that that's the root of the issue. I'm not sure how Clash could carry on after encountering this error, from the looks of it is simpler just fixing this bug than try to figure out how to carry on :-). |
|
A patch that manages to carry on would treat the error case as though the case statement had been provided with the most trivial instance of a non-error case imaginable maybe involving some number like 0x12345678 that I could later search for. Or 0xdeaddead. Regards PTB |
|
Actually we shouldn't have believed my success with 1.1.0 but further testing shows it was right, though I was wrong to say so. The issue is that was on a 32-bit machine so Word -> Signed 32 does not involve a size change whereas on a 64-bit machine it does. But I retried with Word -> Signed 16 and it still fine so the result should be believed. On the 64-bit machine running 1.6.2 I have tried replacing fromIntegral for Word -> Signed 32 with unpack . resize . pack and it synthesizes fine. I have also tried replacing Word -> Signed 32 with Word -> Signed 64 with fromIntegral still the implementation in 1.6.2 and that still errors. So the problem seems (superficially) to be with fromIntegral in 1.6.2 . There is precisely ONE use of fromIntegral in my 50,000 lines of code. I'll try replacing it and report. |
|
It's caused by this VHDL specific primitive introduce in 2e4096a It requires its argument to be a If only he had an equivalent of |
|
Can you do that by assigning a new identifier for the Expr argument of fromInteger, during the translation? |
|
I've eliminated explicit uses of fromInteger in the code but something must remain because I'm still getting a (different) error at that same point. This time by sheer luck a NOINLINE indicates which module this is in and I should be able to narrow it down. It still seems to be unhappy about precisely wordToInteger :
If we take the warnings as Gospel truth, it is worried about primitives for The warnings are all "Dubious primitive instantiation for ...: ...: Integers are dynamically sized in simulation, but fixed-length after synthesis. Use carefully" Regards PTB |
|
(partially) reversing patch 2e4096a allowed vhdl synthesis to complete with 1.6.2.
(I used a bigger computer than before! That is why it is about 10x faster). The patch was introduced July 18 2021 and the working 1.5.0pre I have been using until 1.6.2 was built July 7, so by luck I happened to have more or less the last build without this particular bug, which explains some things. The patch introduces Primitives/Sized/Signed.hs as backing for a change in prims/vhdl/Clash_Sized_Internal_Signed.primitives that modified the behaviour of Clash.Sized.Internal.Signed.fromInteger# . I don't understand that change (it's commented of course but the comment needs more to ping my understanding) but I reversed it. I was not able to drop Signed.hs as something else seems to reference it somewhere else in 1.6.2. I couldn't find what. Yes, I made the reversal in the .primitives.yaml file instead of the .primitives file as those are no longer what are used nowadays, but that's minor. (the semantics was "VHDL generated forSigned.fromInteger now truncates, like the Clash simulation, when the result is smaller than the argument" ..... that must be evoked in Word64 -> Signed32 or similar, and how Integer gets into it I don't know, but smaller probably means shorter in bits, not smaller in value, and it is saying to just truncate instead of anything else, but WHAT else is it that was done and no longer should be done but that I have resuscitated? Was/is the sign bit retained?) |
Previously it could only handle Identifier. (And Literal, which is handled seperately in Clash.Backend.VHDL.expr_) I've also renamed it to make it clear this blackbox is VHDL only. Fixes #2149
Previously it could only handle Identifier. (And Literal, which is handled seperately in Clash.Backend.VHDL.expr_) I've also renamed it to make it clear this blackbox is VHDL only. Fixes #2149
|
Of technical interest only - but I was not able to pull Primitives/Sized/Signed.hs even though it had been newly introduced in the patch that I reversed because there is a purely formal reference to its fromIntegerTF introduced later in Clash/Driver.hs, as part of the knownTemplateFunctions table. |
|
I'll try to explain what's going on. Integers in haskell are unbounded in sized. Because Integers are unbounded in Haskell and bounded in HDL this can create differences in behavior between the generated HDL and simulations run in Haskell, when these Integer values under/overflow in HDL. The
Previously the VHDL implementation incorrectly preserved the sign when shrinking too. Footnotes
|
|
Thanks for the explanation Leon! Perhaps you could break down fromIntegral :: Word -> Signed 32 into parts so I can see how the fromInteger and its translation to resize() appeared in its synthesis before the patch replaced that. Tx PTB |
|
We've released v1.6.3, which includes a fix for this issue. |
|
Thank you Martijn The colleague who told me about it got a tycon error, however. It sounds to me like a residue of a previous compilation.
That;s impossible. So we'll figure what's up later. I might be interested in making all the extra warnings go away. My guess is that Int, not Integer, is involved as the variable sized thing it does not like.
I wrote the code on a 32 bit machine originally, and later moved it to a 64 bit machine, and I would have done the port by making things like Word over to a fixed length Word everywhere. One can be pretty sure I wouldn't have left anything that can change size by context hanging around ... with the excepton of Int, which is used as an indexing type in so many functions and intermediates (e.g. fromEnum) that I decided to leave it be. I remember consciously deciding that moving to fixed size Int wouldn't help as I'd still need it to engage with all the library functions like shiftL. Could that be the source of those warnings? They don't issue in 1.5.0pre. Regards PTB |
|
Those warnings appear in designs when you have / use functions which go via So in terms of what's happening during compilation, nothing has changed - Clash is now just more vocal about it. We've already made some more black boxes to reduce these warnings (see #2066). |
|
There is no instance of "Integer" anywhere in the code proper, but there is in the extra TestBench code base that forms part of the test rig, although it should not be in the compiled TestBench executable because it is used in the Elf loader that builds the insides of a PROM (from an Elf executable file) for use at simulation time, and only the PROM should appear in the synthesized code. Is there something I could look for in the VHDL? Thanks PTB |
|
You can mark test benches with {-# ANN myTestBench (TestBench 'entityBeingTested) #-}and it should suppress the primitive warnings |
|
I removed all instances of Integer everywhere (it was only used in the Elf loader as a union type of Word 32 and Word 64, so I aliased it to Word and goodbye and good luck to anyone who wants to cross-compile - now it'll always be expecting 64 bit Elf on a 64 bit compile platform, etc). Still the same warnings. Thanks again PTB |
|
Ah, my mistake on that annotation usage: I got the order of the names mixed up. A better example from our documentation is this: f :: Bool -> Bool
f = -- some entity
{-# ANN f (defSyn "f") #-}
{-# ANN f (TestBench 'g) #-}
g :: Signal Bool
g = -- some test benchThe annotation tells Clash With regard to removing |
|
OK ... am I right in that you are saying g is the thing I am synthesizing, comprising a test bench composed of a test rig connected with f, the thing notionally being tested?
That would be "g". |
@alex-mckenna You did get it right the first time. The example in the documentation is wrong. So the example should be: f = -- some entity
{-# NOINLINE f #-}
{-# ANN f (defSyn "f") #-}
{-# ANN g (TestBench 'f) #-}
g :: Signal Bool
g = -- some test bench testing fAnd you don't need an extra Synthesize1 annotation on Footnotes
|
|
I quite like the explicitness of not naming it {-# ANN myTestBench (TestBench 'entityBeingTested) #-}I'll soon fix this. (I could have sworn we already had. I actually did swear when I noticed we had not.) |
This follows on from issue #2126 . Christiaan's patch cured that bug, but a different issue arises later on in the vhdl synthesis with 1.6.2 (works fine in some 1.5.0pre) that is also deadly. I presume it happens during netlist generation, but it may be just prior ... or after. Christiaan asked me to re-run with a patch that produces more error reporting, and this is the output:
That is from this line in the patched source (I'll fix the indenting later):
Regards
PTB
The text was updated successfully, but these errors were encountered: