<modwizcode>
Yeah so wq mentioned to me at some point that clean is somehow broken, but I didn't dig into it, been crazy busy.
<Sarayan>
Now I'm not doing anything... and it asserts :-)
<Sarayan>
Yeah, this start of year is kinda insane at times
<modwizcode>
Let me pull up some code real quick
<Sarayan>
and it's only the second week
<modwizcode>
I've just been trying to cram all my job stuff in right before classes truly start and I've not had much energy/time for much else.
<modwizcode>
Sarayan: I need a micromem.rom?
<modwizcode>
looks like I can stub it out
<Sarayan>
Sorry, added
<modwizcode>
thanks
<Sarayan>
idn't realize it was still using it after the sv2v, but it's logical
peeps[zen] has joined #nmigen
peepsalot has quit [Ping timeout: 240 seconds]
Bertl is now known as Bertl_oO
bvernoux has joined #nmigen
<modwizcode>
Solved it lol.
<Sarayan>
oh? cool
<modwizcode>
Sarayan: use -g3 with write_cxxrtl until it's fixed.
<modwizcode>
I submitted a PR tho
<Sarayan>
really? fun
<modwizcode>
It'll not have debug info for all public wires tho
<Sarayan>
I'll apply the patch instead :-)
<modwizcode>
fair enough :)
<Sarayan>
I mean, it's not really a massive one :-)
<Sarayan>
damn, the output is still... problematic
<Sarayan>
maybe you'll have an idea. Look for eCntr (it's a 4-bit lfsr counter that is used to generate a /10 clock)
<Sarayan>
the conversion result in m68k.v seems reasonable
<Sarayan>
but the .cc only has one line that reads it and nothing that writes to it
<Sarayan>
same for p_E, the resulting divided clock, read once, nothing writes it
<Sarayan>
am I misreading the verilog and they're broken, or did the optimization just broke it?
<modwizcode>
Hmm
<modwizcode>
I'll look a bit further. I didn't check that the optimization pass was fully without errors, I tracked down the reason this was happening to that. Let me try and figure out what's going on.
<Sarayan>
I'm trying different O levels now to check
<modwizcode>
If you run with g0 (no debug anything) the wire is still generated but it's never assigned
<Sarayan>
I'm trying -O0, clang++ is still compiling
<Sarayan>
...and E is not workin
<Sarayan>
working
<modwizcode>
E?
<modwizcode>
oh
<Sarayan>
og.kervella.org/main.cc for the main code
<Sarayan>
how course my main.cc may be bad too, there's no reason to have only one bug :-)
<modwizcode>
Even at O0 that eCntr isn't updated ever in the cxxrtl output
<Sarayan>
yeah
<modwizcode>
Uh
<modwizcode>
Hmm
<modwizcode>
It might have something to do with how it's being assigned
<modwizcode>
I've never worked with verilog that uses posedge on a vector of clocks that's... unique to me
<modwizcode>
I'm going to look at the rtlil to see what yosys made of it after proc pass
<modwizcode>
The rtlil looks good
<modwizcode>
I think you should post another issue on yosys for this
<Sarayan>
You think? Ok then
<modwizcode>
I think I might see the issue
<modwizcode>
but I'm not sure I thought cxxrtl acts conservatively and handles this anyway
<modwizcode>
but it might not be handling the fact that the clocks are slices
<modwizcode>
it definitely is supposed to handle that, but it might not be handling that correctly
<modwizcode>
It works if I assign it to a wire first and use the wire in the @posedge instead
<Sarayan>
Issue #2542. Wife is hungry, need to take care of that
<modwizcode>
hah
<Sarayan>
pasta is cooking :-)
<Sarayan>
thanks for tracking these bugs btw
<Sarayan>
in the original sv code it's a 5-logic struct
<Sarayan>
// Clocks, phases and resets
<Sarayan>
logic clk;
<Sarayan>
logic pwrUp; // Asserted together with reset on emulated system coldstart
<Sarayan>
typedef struct {
<Sarayan>
logic extReset; // External sync reset on emulated system
<Sarayan>
logic enPhi1, enPhi2; // Clock enables. Next cycle is PHI1 or PHI2
<Sarayan>
} s_clks;
<modwizcode>
Ah that... almost kind of makes sense
<modwizcode>
I've never written system verilog before. I still don't know how I feel about clocks being in a struct.
<modwizcode>
Especially considering how the enables aren't actually clocks. Converting it to an array/vector makes sense in light of that form though.
<Sarayan>
yeah
<Sarayan>
it's becoming a stylistic issue, not pure insanity :-)
<modwizcode>
I have to use const_cast to make it so I can log things during debugging because some stuff isn't const safe (most of that should probably be fixed but it's deep enough in yosys that it's like it's own issue)
<d1b2>
<DX-MON> that definitely should be fixed as use of const_cast to erase const-ness is UB and so always a bug
<d1b2>
<DX-MON> (the cast exists to make such code smelly for an easier review)
emeb_mac has joined #nmigen
<modwizcode>
The const cast is for me to debug without solving the issue, it needs to be fixed for real at some point.
<d1b2>
<DX-MON> aye - hence only a mention in agreement that it should indeed be an issue raised to be fixed later 🙂
<modwizcode>
yep.
<modwizcode>
I always forget what const_cast is actually for lol
<d1b2>
<DX-MON> its only purpose in life is as a code smell for casting away const
<d1b2>
<DX-MON> there is no way to cast away const that is defined by the standard - it is always UB
<modwizcode>
I think I remember
<modwizcode>
hmm actually nevermind
<modwizcode>
I thought it was safe as long as the object wasn't like originally defined const. But like that doesn't sound safe at all so I doubt that's right.
<d1b2>
<DX-MON> the standard only takes into consideration object-constness in the scope the cast is being performed
<d1b2>
<DX-MON> and for it to be necessary, it has to be to cast const away which is UB
<modwizcode>
Leave it to c++ to define something that's only use is UB
<modwizcode>
I see where this issue is happening now, I don't know enough about yosys to understand if this is actually an issue with the definition of "is_wire" on sigspec. it only returns true if there's only 1 chunk and that chunk has the same size as the wire it's connected to. Which means that a smaller portion of a signal (like a sigspec that's a single bit of the wire) will not.
<d1b2>
<DX-MON> ahahahaha, yeah.. technically more than one something as most uses of reinterpret_cast<>() are also UB
<modwizcode>
Yeah I wish they had just defined that one to do the right thing (or added a seperate cast as I've seen elsewhere)
<modwizcode>
The only "safe" way to express easily that you want to do a non-byte read of a byte area is to use memcpy into like an int and assume it's optimized, and even that is questionably defined by the spec I think.
<modwizcode>
but it's such a common thing and useful ability that it should be defined somehow.
<d1b2>
<DX-MON> that is the correct way to do it, and it is defined by the spec (they're working on solidifying that actually.. which is why std::byte was introduced)
<d1b2>
<DX-MON> and also, on all reasonably modern (within the last decade..) compilers, memcpy() is an intrinsic first before the CRT call so it is agressively optimised out
<modwizcode>
I thought if you read the spec it's kinda wishy washy about if that's safe or just unspecified rather than undefined. But it's been awhile since I wasted a day trying to nail that down.
<d1b2>
<DX-MON> *aggressively
<d1b2>
<DX-MON> I'm going on my own reading of the spec combined with the advise and wisdom from committee members who presented on this exact topic at CppCon 🙂
<modwizcode>
Oh for sure. I personally just feel like it's pretty unclear compared to a defined way like a specific cast. Iirc the proposed standard would add a specific cast for this I think?
<d1b2>
<DX-MON> yeah, there is a byte cast (byte_cast<>() even I think) being proposed
<d1b2>
<DX-MON> hopefully for inclusion in C++23
<modwizcode>
Nice
<d1b2>
<DX-MON> because the question had come up of "how does one define memcpy() in pure C++ then?".. and they realised the standard doesn't exactly give a way to do it..
<d1b2>
<DX-MON> hence std::byte and now also the cast
<modwizcode>
I wonder if rust has a defined way to do that kind of thing too, it seems somewhat unrust-like to alias data in any way but it's a necessary thing. Maybe they just rely on llvm to be really good about recognizing the operations that encode it?
<d1b2>
<DX-MON> which well.. broke the idea that "C++ can and should be self-contained without any external dependencies to make it functional".. when clearly "use memcpy()! (but you can't write it in C++)" violates that by forcing an unwanted dep on C
<modwizcode>
Yeah
<modwizcode>
But like I don't think it's defined in C either lol
<modwizcode>
but I think C has less rules anyway so it matters less
<d1b2>
<DX-MON> uh.. it wierdly is actually thanks to the same reason you can have a union of int and float and assign to the float then access the int
<d1b2>
<DX-MON> it's nasty and C++ closed that loophole with the strict aliasing rules along with only reads from the active member of the union being defined
<modwizcode>
That's defined for C?
<modwizcode>
I didn't realize that.
<d1b2>
<DX-MON> yeah, that's defined for C
bvernoux has quit [Quit: Leaving]
<modwizcode>
Getting access to the C spec is more annoying. Technically both specs are annoying but the C++ spec is basically viewable via cppreference.com
<d1b2>
<DX-MON> you can get the final drafts for both sets of specs.. which are the same thing as the ISO document, but without the ISO copyright
<d1b2>
<DX-MON> but yeah.. agreed that it's unpleasant
<modwizcode>
One day maybe C and C++ will both define enough of the details that you can actually use the spec to design an OS without relying on compiler specifics.
<d1b2>
<DX-MON> https://eel.is/c++draft/ is also a complete reference of the C++ standard in its current draft form
<modwizcode>
Also parsing through the standard itself is a pain, I'm glad that website exists bc otherwise figuring out specifics is like an hour long affair
<d1b2>
<DX-MON> agreed that cppreference is one of the best resources for this stuff out there.. the write-up on reinterpret_cast<>() for example is phenomenal for understanding the subtitles of the cast and why most of the time it's UB except in some very particular cases when it's not
<modwizcode>
Yeah it's very detailed
<modwizcode>
although sometimes now I feel like it's too detailed to the point of obscuring the useful information.
<d1b2>
<DX-MON> yeah, that's fair
<modwizcode>
Oh there's some weird const casting happening in yosys already
<modwizcode>
uh wtf
<modwizcode>
oh well not my problem
<modwizcode>
I've never seen someone take a const class method and then cast this to a non-const pointer.
<d1b2>
<DX-MON> for the more advanced topics it's definitely "awesome writeup, but.. uh.. could I please have a summary version of the key points for those less able to handle standardese, or in a hurry please?"
<d1b2>
<DX-MON> that sounds like the pointer should be const return_t (*)(params_t...), while I bet it was written as return_t (*)(params_t...)
<d1b2>
<DX-MON> well.. I've been wanting to contrib more to the FOSS FPGA tool community for a while so it sounds like the kind of improvement/patchset which I excel at and might and up taking on as a "my first yosys contrib"
<d1b2>
<DX-MON> (that's not to say nobody else touch it.. far from that.. just that if nobody else does, I might well turn my eye to it)
<modwizcode>
But like the issue seems to be that sometimes you can have values that are "packed" and before you can read data you have to unpack them. But the read methods are const safe. The whole thing seems like a mess because I'm positive that C style cast is equivilent to the const_cast which is UB
<d1b2>
<DX-MON> it will most probably be being compiled as a const_cast<>() but C style casts are strongly discouraged in C++ specifically because there are no guarantees of that
<d1b2>
<DX-MON> and yeah.. unpacking etc seems like something that should be completely doable in a const-marked function so.. some annotation needs doing to allow that code to be clenaed