[U-Boot] [PATCH 3/3] MIPS: Abstract cache op loops with a macro

[Paul Burton]

On Fri, May 27, 2016 at 04:36:21PM +0200, Marek Vasut wrote:
> > The problem is that then both that function & its callers would need to
> > know about the types of cache ops, and there'd need to be some mapping
> > from flags to the actual cache op values (of which there'll be a couple
> > more once L2 support is added).
> 
> But the callers already know which ops they must invoke, right ?
> You can just have enum {} for the bit definitions. It's all local
> to this file, so I don't see a problem.

It's all local to the file, but it's still an extra level of abstraction
that I don't see the value of. It just means that both callers & callee
have to know about various cache ops, and I don't see the point in
duplicating that knowledge. I think it's much cleaner to keep that
knowledge in one place - the caller.

> > I think it's much simpler & cleaner to
> > just go with the macro in this case - it keeps knowledge of which cache
> > ops to use with the callers, and it ends up generating the same code as
> > before (ie. the inner loop gets unrolled to just the relevant cache
> > instructions).
> 
> I would expect GCC to take care of inlining the code, but you can always
> use the always_inline directive if that's your concern. Even cleaner way
> might be to use CONFIG_CC_OPTIMIZE_LIBS_FOR_SPEED and compile the
> cache file with -O2 .

It may well inline it, remove the flags indirection & generate the same
code - I haven't tried it to find out. Making the code less clear with
that abstraction, even if the compiler will come along & clean it back
up, doesn't make much sense to me as someone who has to read the code.

I understand the preference for functions over macros, but I think this
is a case where the macro has strengths that a function cannot provide
(ignoring perhaps varargs which would be even more messy).

Thanks,
    Paul