Inlining of functions...

Go To Last Post
10 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi,

I have a function:

static inline void set_bit (unsigned char port)
{
  unsigned char bv; 
  bv = _BV (port & 7);
  switch (port & ~7) {
  case PB: PORTB |= bv;break;
  case PC: PORTC |= bv;break;
  }
}

Of course I've defined PB to be "8" and PC to be 16 etc. Of course in reality I support more ports than just PORTB and PORTC.
Then I define say: LED1 to be pB7 which is (PB | PB7). Now I can:

set_bit (LED1);

You might think that this is a lot slower than just doing PORTB |= _BV (PB7);, but the compiler will surprise you: Even with a whole bunch more entries in the "switch". Anyway.... turning on "LED1" compiles to:

        sbi 37-32,0

Nice!

The problem is.... I have a very similar function clr_bit:

static inline void clr_bit (unsigned char port)
{
  unsigned char bv; 
  bv = ~_BV (port & 7);
  switch (port & ~7) {
  case PB: PORTB &= bv;break;
  case PC: PORTC &= bv;break;
  }
}

and this DOES NOT get inlined.... So for each bit I get:

        ldi r24,lo8(11)
        call clr_bit

Which takes a significant amount of clocks more than the "optimized" version....

Why does my set_bit get inlined as I asked it to, but clr_bit not?

I should sacrifice to the big teddybear: -finline-limit=n
seems to be my friend. It's not my program. Different source to work on today. I'll have it tested tomorrow if it does as I wish... :-)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Try this:

static inline void clr_bit (unsigned char port)
{
  unsigned char bv;
  bv = _BV (port & 7);
  switch (port & ~7) {
  case PB: PORTB &= ~bv;break;
  case PC: PORTC &= ~bv;break;
  }
}

Untested. But it is more similar to your set_bit()

Personally, I dislike these machinations.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
__attribute__((always_inline))

*MAY* force inlining (untested - may not apply to avr-gcc).

For more __attribute__ fun see:

http://gcc.gnu.org/onlinedocs/gc...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

> *MAY* force inlining (untested - may not apply to avr-gcc).

It does.

Jörg Wunsch

Please don't send me PMs, use email if you want to approach me personally.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Personally, I dislike these machinations.

I agree. Generally the low port letters on an AVR8 can be reached with single SBI/CBI. As there are no runtime parameters for these op codes, any "generic" approach will create complex sequences of instructions at best, and at worst introduce hidden RMW problems.

Cliff and I and probably others have opined in the past: Will e.g. LED1 really change positions at runtime on any particular board layout? Unusual. If indeed you are on a quest for near-optimal code sequences then skip the generic routine.

So what I'd do is to create LED1_ON(), LED1_OFF(), LED1_TOGGLE() macros that will (typically) end up to be SBI/CBI. Change the board layout and you only have to change the macros.

Or you could also have them be parameterized macros and pass the port and bit. Still done at compile time.

First option:

#define LED1_ON() (PORTB |= (1 << PB7))
#define LED1_OFF() (PORTB &= ~(1 << PB7))
#define LED1_TOGGLE() (PINB |= (1 << PB7))

Second option:

#define LED1_PORT PORTB
#define LED1_PIN PB7
...
#define SETIT(theport, thepin) (theport |= (1 << thepin))
 (similar for clear & toggle)
...
        SETIT(LED1_PORT, LED1_PIN);

Now you don't need to worry about calls and switch() and inline functions. And you still define in one place the configuration on this particular board.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
Second option:

#define LED1_PORT PORTB
#define LED1_PIN PB7
...
#define SETIT(theport, thepin) (theport |= (1 << thepin))
 (similar for clear & toggle)
...
        SETIT(LED1_PORT, LED1_PIN);

Now you don't need to worry about calls and switch() and inline functions. And you still define in one place the configuration on this particular board.

Better yet:
#define xpaste(a,b) a ## b
#define paste(a,b) xpaste(a,b)
#define setit(target) (paste(target, _PORT) |= (1<< paste(target, _PIN)))

setit(LED1);

The messy stuff only has to be written once.
For my own work, the LED1-specific macros would be LED1_LET and LED1_BIT .

Regarding inline functions, I would prefer the attribute method to twisting my expressions.

Moderation in all things. -- ancient proverb

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I generally dislike these things, also, but...

set_bit and clr_bit do make it clear what you intend to do (the documentation "issue")

On the other hand, in your mind, you still have to associate a bit with the external function (an LED, for example). So, I tend to use Lee's first construct, LED_ON(), etc, when I am moved to do so.

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For Lee's 1st option it should be :

#define LED1_TOGGLE() (PINB ^= (1 << PB7))

1) Studio 4.18 build 716 (SP3)
2) WinAvr 20100110
3) PN, all on Doze XP... For Now
A) Avr Dragon ver. 1
B) Avr MKII ISP, 2009 model
C) MKII JTAGICE ver. 1

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I agree that using things like "LED1_ON()" is more efficient.

I have some code that does it one step nicer than you guys are suggesting.

I make a file that has the pin configuration, and call it .pins . For just "led1" it would look something like this:

PB4    LED1

Just portname, logical name.

Next I have a program that turns this into a headerfile .h that defines LED1_ON(), LED1_OUTPUT() and everything else you'd ever want.....

That said....
These set_bit and clr_bit functions are NOT less efficient than the LED1_ON() macro, as long as the pin name is known at compiletime. The compiler will optimize everything else away and just leave the exact same assembly code as "LED1_ON()" would.

They ARE less efficient if you do not know which pin to use at compile time. But for non-time-critical code, it's useful. Suppose I have 16 outputs, that are spread out over 3 different ports, which more or less need to be addressed as output 0... output 15. The code in the AVR doesn't know what function each pin has. External commands will tell it to make a pin high or low. Now what?

This is where set_bit and clr_bit come in.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Now what?

Yes, indeed, a conundrum with the AVR instruction set. It is not unusual to run into the situation on Mega88-class apps--only one "full" port (D) and that full port not available when using the USART. Mega164-class and larger pin-count usually at least part can be done on full ports. Generally, I just unroll them and do them separately.

// **************************************************************************
// *	O U T P U T   C O N T R O L
// **************************************************************************
//
//	Active-high outputs

	if (valve_mask & VALVE_1)
		{
		OUT1_ON();
		}
	else
		{
		OUT1_OFF();
		}

...
	if (valve_mask & VALVE_16)
		{
		OUT16_ON();
		}
	else
		{
		OUT16_OFF();
		}

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.