search

PaulStoffregen / cores

Teensy Core Libraries for Arduino
507 stars 372 forks source link

Possible need of "volatile" with dsb/isb instructions #634

Open ssilverman opened 2 years ago

ssilverman commented 2 years ago

I was reading this: https://forum.pjrc.com/threads/69288-Strange-IRQ_CMP0-behaviour-of-Teensy-3-2-help-needed-to-track-down-issue?p=298177&viewfull=1#post298177 and remembered I've seen asm("dsb") instructions without the volatile.

The following places in this repo use this instruction without volatile:

  1. https://github.com/PaulStoffregen/cores/blob/fdfd18f935fac6859d19154e4c11f869e685b7d4/teensy4/usb_serial.c#L343
  2. https://github.com/PaulStoffregen/cores/blob/fdfd18f935fac6859d19154e4c11f869e685b7d4/teensy4/usb_serial.c#L388
  3. https://github.com/PaulStoffregen/cores/blob/2e2dcf7c41c3096650aa528795d761d8e1e7ed91/teensy4/startup.c#L278 (there are two "dsb" and two "isb" here)
  4. https://github.com/PaulStoffregen/cores/blob/a2368ad57e9470608a234d942c55a2278c6cd72b/teensy4/AudioStream.cpp#L335
FrankBoesing commented 2 years ago

As these things work, there is no need to change them.

FrankBoesing commented 2 years ago

On the contrary - I'd think about removing the DSB  (for example for audiostream), after a test. 
A unneeded dsb is esp. with volatile a real performance killer.

"voaltile" by nature kills performance :)

ssilverman commented 2 years ago

Is there no chance of reordering, then, even with future compilers? Or maybe even the lines with :::“memory” can’t be reordered?

FrankBoesing commented 2 years ago

don't know.

tsandmann commented 2 years ago

There's no difference in using volatile here, because no output operands are used:

asm statements that have no output operands and asm goto statements, are implicitly volatile.

See gcc docs.

Reordering is a completely different story (that's independent of volatile).

ssilverman commented 2 years ago

Thanks, @tsandmann. That makes it a little more clear. Is there even a way to disable reordering for these statements?

Update: or maybe the compiler considers the “dsb” instruction non-reorderable?

FrankBoesing commented 2 years ago

There is no difference, yes, so it does not hurt to just leave it there :)

FrankBoesing commented 2 years ago

...and was it for GCC 5.4 the same?

tsandmann commented 2 years ago

Thanks, @tsandmann. That makes it a little more clear. Is there even a way to disable reordering for these statements?

Did you see any issue because of reordering there? I don't think there's an easy way to guarantee that, but if it's an actual issue here, sth. like the example in the linked docs should work.

Update: or maybe the compiler considers the “dsb” instruction non-reorderable?

I don't think so, but adding ::: "memory" prevents reordering any read/write instruction around it.

tsandmann commented 2 years ago

...and was it for GCC 5.4 the same?

According to gcc-5.4.0/gcc/Extended-Asm.html, yes.

FrankBoesing commented 2 years ago

thanks!

ssilverman commented 2 years ago

@tsandmann no issue with reordering that I know of. I thought that the comments in that forum post suggested that one must add volatile to prevent reordering. There’s some finer details which you pointed out. I’m just trying to learn more. Thanks for the info. :)