Improve generated Rust code

[Gui-Yom]

• Rust vector backend
• Do not use address offsets when accessing buffers (gRemoveVarAddress)
• Zero-initialize stack arrays
• No () for single term expressions
• Render ForLoopInst as for ranges
• Fix incorrect assertion in the DeclareVarInst visitor in struct_manager.hh
• Fix inconsistent formatting in various files
• Correctly generate slice type
• Remove usage of libm
• Simplify boolean conditions
• Intermediate cast to an integer when casting a bool to a float

[sletz]

Thanks. You'll have to check:

• that current Rust tests are OK (which is not the case right now), in tests/impulse-tests and using make rust -i
• add new tests for the -vec mode, in https://github.com/grame-cncm/faust/blob/master-dev/tests/impulse-tests/Makefile#L357

[Gui-Yom]

Last changes includes :

• Put the repr(C) attribute under a feature gate (Set repr(C) on the Dsp struct to recover performance #889). Through benchmarking I can't seem to prove reprc(C) is necessarily better.
• Complete rework of the main dsp loop by using a chunk iterator (inputs[0].chunks(vsize) & outputs[0].chunks_mut(vsize)). This eliminates the need for a last pass with the remaining samples (when sample count isn't a multiple of vsize), the Rust compiler understands chunks very well and generates optimized assembly for us.

I'll try to make impulse tests work.

[sletz]

Thanks. Tests are OK in solar mode. But faust -lang rust -vec foo.dsp is crashing here.

[bluenote10]

Please don't put repr(C) under a feature flag. My understanding from the internals discussion is that should always have used repr(C) to begin with. The performance regression is visible basically in all my DSPs models, and it is really significant. You could reproduce it either with the example I posted in that discussion or with the benchmarks here.

[sletz]

Recompiling current version, I get:

[ 42%] Building CXX object CMakeFiles/faust.dir/Users/letz/Developpements/faust/compiler/generator/instructions_compiler1.cpp.o
/Users/letz/Developpements/faust/compiler/generator/instructions.cpp:167:25: error: out-of-line definition of 'DeclareBufferIterators' does not match any declaration in 'DeclareBufferIterators'
DeclareBufferIterators::DeclareBufferIterators(const std::string& name1,
                        ^~~~~~~~~~~~~~~~~~~~~~
[ 42%] Building CXX object CMakeFiles/faust.dir/Users/letz/Developpements/faust/compiler/generator/instructions_compiler_jax.cpp.o

[sletz]

many tests in -vec mode don't work. Is the PR still stable enough to improve the scalar mode? Or should I wait before merging?

[Gui-Yom]

The issue with the main loop epilogue has been fixed, though the solution is not very elegant.

Failing tests with -vec -vs 4 :

• constant
• delays
• pitch_shifter
• thru_zero_flanger
• zita_rev1

Because of an unknown error in codegen :

ASSERT : no vector name for : IN[0]
ASSERT : please report this message, the stack trace, and the failing DSP file to Faust developers (file: C:\W\PR\faust\compiler\generator\dag_instructions_compiler.cpp, line: 438, version: 2.60.4, options: -a archs/rust/architecture.rs -lang rust -i -ct 1 -es 1 -mcd 16 -double -ftz 0 -vec -lv 0 -vs 32 )
make[1]: *** [Make.rust:115: ir/rust/constant.ir] Error 1

Additional failing tests with -vec -vs 32 :

• reverb_designer
• spectral_level
• virtual_analog_oscillators

Because of an incorrect struct variable access :

error[E0425]: cannot find value `fRec29_tmp` in this scope
    --> src\bin\virtual_analog_oscillators.rs:5766:54
     |
5766 |                 self.fVbargraph14 = fSlow41 + 2e+01 * F64::log10(fRec29_tmp[(i32::wrapping_add(4, ...
     |                                                                  ^^^^^^^^^^ help: you might have meant to use the available field: `self.fRec29_tmp`

I'll try to fix those asap.

---

Also @bluenote10, I investigated a bit about the impact of repr(C) (see some preliminary benchmark results here : https://github.com/Gui-Yom/faust-rs). While it seems to always result in a win in scalar mode, using it on the struct generated with -vec can sometimes result in a loss. The reprc(C) attribute should definitely not be under a feature flag though but instead a faust cli flag.

[sletz]

Cleanup and merged, thanks!

[bluenote10]

The reprc(C) attribute should definitely not be under a feature flag though but instead a faust cli flag.

The default should have been "on" though, not "off".

Defaulting to not using it is unfortunate, because one starts to depend on the non-determinism introduced by field re-ordering, which can vary with the Rust version. We should default to something that is deterministic instead, and most likely is more performant. People may not be aware that this issue (and the switch) exists and scratch their heads for a long time like I did when running into this non-deterministic behavior.

[sletz]

Do you mean having gReprC = true; in compiler/global.cpp ?

[bluenote10]

Yes I think so. I haven't read the code in detail, but guess this is where the value comes from if not defined. And the command line switch would then have disabling semantics to explicitly turn it off, i.e., something like --no-reprc or --rust-allow-field-reordering or so.

[sletz]

@bluenote10 or @Gui-Yom can you possibly prepare a PR ?

[crop2000]

@Gui-Yom did you benchmarked the code with that uses the -vec option?