-
-
Notifications
You must be signed in to change notification settings - Fork 424
fix Issue 13138 add peek/poke to core.bitop #892
Conversation
Requires dlang/dmd#3773 |
Your |
Agreed. I suggested |
It's done to make it easy to see what is different about each declaration, and what is the same. Sort of making a table out of it.
??
What'll happen with |
I agree tha peek and poke are the usual names for such function. |
I think @yebblies was referring to indentation, not alignment, in the unit tests. |
We shouldn't guess what @yebblies means, he should clarify. |
I mean you didn't use 4 spaces for indentation, and for some reason you wrote out the code four times instead of using foreach over a typetuple. |
So? Nobody is going to be confused when they see Do you really think we'll get more people coming from basic and expecting peek/poke than we'll get coming from C and expecting volatile?
And the same thing the other way around if they're named peek/poke. That's the beauty of documentation, it explains what the functions do. |
I really, sincerely doubt it. I do fairly low-level programming for my day job and had no idea "peek" and "poke" are names that some people use for volatile memory access. And I mean really no clue. Never before have I seen these terms used for this. If you actually look at the relevant Wikipedia article, it seems like the origin of these terms is very obscure. You're assuming that people who do low-level programming today have dealt with (relatively) ancient 8-bit machines/devices. If I were looking for functions to do volatile memory operations, I would have no clue to look for "peek" or "poke". I think it's much more realistic to expect that people who need these intrinsics are familiar with C, at the very least. It's the de facto embedded language today. I also don't see how the definition of "peek" and "poke" from that Wikipedia article has anything to do with the volatility of the operations, which is the entire point in this case. By the way, Rust went with
Nimrod and Ada also use the term "volatile": |
Ok.
std.typetuple is part of Phobos, and druntime is not supposed to depend on Phobos. Of course, I could copy the definition of TypeTuple from Phobos, but I thought I'd keep the unittests focused. |
It's a one-liner, and will make the unittests more focused. We do this all the time in the dmd test suite.
|
Aligning the arguments I can understand, but aligning the |
Now has tabs! |
By the way, why aren't these generic? I don't see any reason we should limit them to built-in types; LLVM and GCC can both handle arbitrary data types. I assume DMD can too (since this just maps to regular load/store instructions). |
I'm surprised you haven't heard of it: I was intrigued, though, that Rust used the same parameters in the same order as the usual peek & poke. I googled a bit and couldn't find any history on Rust on that issue. Rust calling them that does give the name more legitimacy. I don't care that much one way or the other, but one thing about the long names, though, is:
the expression gets a bit long quickly with the longer names. |
Oops!
Ok.
Yes, because the same parts line up. |
Not exactly. Many types don't have "regular" load/store instructions (like 80 bit types), and I'm unsure what a volatile load of an aggregate type should be. Keep in mind that these are meant as primitives - as simple and straightforward as possible, to make it quick and easy for compilers to implement them and get them right. They are not meant as high level abstractions - those sorts of things belong in Phobos. |
It's not just about aggregate types - what about e.g. signed types? And But FWIW, I think volatile ops for aggregate types should be treated just the same as for primitive types. This is how e.g. LLVM does it, and I'm 99% sure GCC does the same. There is the caveat that they usually won't be done as single instructions in the resulting machine code, but the same goes for 64-bit ops on a 32-bit target that doesn't support 64-bit loads/stores. Edit: To be clear, the same goes for 80-bit reals as for aggregates. |
DMD does not currently have support for templated intrinsics. You can always build them on top of these intrinisics. |
They should be wrapped most of the time, and not appear often in user code. |
BTW, it could be changed to a template in the future without breaking anything, but I kinda like to start out with things being simple and see how they work out. |
Whenevery I want to use core.atomic, I try those names in this order. |
As I posted in the newsgroup loading structure types via peek is bad, cause you end up loading the complete struct even if you only wanted to load a single field. And this a conceptual problem, not an optimization problem. You can load all fields manually with peek/poke but then you can forget the wrapper idea. |
It should be quite possible to create a VirtualStruct!S template which, when given a struct S argument, will generate a type that contains getters/setters for each field that does the correct volatile load/stores. |
Consistency with atomicLoad/Store is a good argument, am convinced. |
I can confirm that these are trivial to implement in LDC and fully support this over DIP62 for interfacing with MMIO and other special registers on MCUs. However, the documentation is still severely lacking. What ordering guarantees do these provide? No reordering w.r.t. all other loads/stores or just volatile ones? etc. It may be fairly obvious what the intended use case requires, but in the light of different optimizing backends, we should try to carefully formalize this. |
Oh, and it might be worth mentioning what the difference between these and atomic{Load, Store} with MemoryOrder.raw is, if any. (I wrote "if any", because |
@klickverbot Can you suggest some appropriate wording? I'm not very good at such formalisms. |
I guess |
volatile is still a keyword. |
Right, let's wait another month? Since volatile is fail what do we do? |
Just merge it as |
@WalterBright I made a pull for your branch that adds a new code.volatile_ module. |
There is no associated dmd pull yet. |
@MartinNowak: The DMD pull request for core.bitop has already been merged (my mistake). |
@WalterBright, can you please voice your decision on this one way or another so we can move forward? |
I'd say somebody else opens the DMD/druntime pull requests for |
See #1032 |
Auto-merge toggled on |
Please write a change log entry describing the newly added functions. |
fix Issue 13138 add peek/poke to core.bitop
@WalterBright: I take this is a commitment to making |
It has been deprecated for ages and will be an error in 2.067, so we could remove it in 2.068. |
Yep, sounds great. All I care about is that this doesn't again stall for no good reason. |
Anyone up for the changelog? |
I was about to edit the changelog, but I couldn't figure out where it should go. Should I add it to the 2.067 branch or to head. If someone can add the 2.067 template to the changelog, I'll make a go at adding this feature to it. |
Great, it should go here, because 2.067 will be the next release. https://github.com/D-Programming-Language/dlang.org/blob/615d729e3076e4c79d35f61c3abf05526ae3afc5/changelog.dd#L5 |
|
https://issues.dlang.org/show_bug.cgi?id=13138