I have made an attempt at fixing an AVR related bug.  Since this is my first attempt at messing with the compiler, I would like a review and critique of my attempt.

The bug seems to be caused by the absence of the MUL instruction for the limited AVR subarchitectures (avr1 - avr35). As far as I could see there is no fallback code for the case where CPUAVR_HAS_MUL is not defined for an 8 bit MUL operation.  I have copied the code for the corresponding 16 bit MUL branch, then copied and adapted the pascal code for fpc_mul_xxx etc in rtl/inc/generic.inc and added declarations to rtl/inc/generic.inc.

Can anyone see any problems with this approach, or perhaps suggest a better approach to fix this bug?

Thanks

Hi,

I don't know a lot about AVR, and haven't actually tried your patch. But I took a things I see: I wouldn't do IFDEF CPUAVR in inc/generic and inc/compproc. Use maybe IFDEF CPU8 and/or CPU16 instead. This code is indeed generic, and will be useful on other limited CPU archs as well. Or just follow the structure of other MUL helpers, and don't ifdef based on the CPU linker will (should) optimize away this code on architectures where it is unnecessary.

Also, instead of copypasting the helper function call code, I'd think about generalizing it, and extracting it to a private method of tcgavr, but as I'm not the maintainer of the AVR cg, i'm not sure this is what they would want there. :)

But in general I'd say it's the right approach, at least we used this elsewhere too.

Charlie

Thanks for the suggestions Karoly. I will remove the ifdefs, this makes the code a bit cleaner.  I don't understand the details of tcg(avr) so it would be challenging for me to try refactoring code.

Christo wrote

On Wed, 2017-09-20 at 12:36 +0200, Karoly Balogh (Charlie/SGR) wrote: >

A complication I've noted is that enabling overflow checking doesn't call the fpc_mul_byte_overflow function (as the naming convention suggested), rather the generated code calls fpc_mul_byte and then check the carry flag to determine whether an overflow occurred. Since the function in generic.inc is plain pascal code there is no guarantee that the carry flag will be set if an overflow should have occurred.  I assume that at a higher level the code generator assumes that a hardware MUL instruction _will_ be used, leading to incorrect overflow detection logic if a software helper is used.

A similar situation exist for the current 16 bit software MUL case with overflow checking.

Any pointers on where to start looking to try and adapt the overflow checking code generation?

implementing 8*8 bit multiplications as library functions is certainly one way to cope with the absence of mul/mulsu instructions in  the "basic" avr subarchitectures, and the resultint compiler error message when confronted with any source code resulting in such a multiplication.

The question is, if that is the most reasonable way to do it, or if it would be simpler to have the compiler directly insert the required compare/add/shift sequence. This should be answered by someone with a very good knowledge of the AVR code generator.

AVR has no working overflow checking yet. Basically, tcg.a_op_reg_reg_reg_checkoverflow and tcg.a_op_conbst_reg_reg_checkoverflow need to be overridden and they have to set ovloc properly which is checked later on by g_overflowCheck_loc,