[PATCH]: finish .cpu support for ARM

Renato Golin renato.golin at linaro.org
Wed Dec 3 11:18:31 PST 2014


On 3 December 2014 at 18:52, Roman Divacky <rdivacky at vlakno.cz> wrote:
> Can you explain why you think testing all variants is useful? Note that
> the switching depends on already existing (and I think already tested)
> components.

That's the problem, it doesn't. There are not many other tests, all of
them too similar to yours to make a difference. Basically, you're
adding two things:

1. Validating the CPU name, which is a small change and you have a
test for that.
2. Adding the feature bits, which has nothing to do with your tests
and are being included.

This is a new behaviour, and one that changes the semantics of
assembly files quite considerably. Have you read the bugs I sent you?
You should also have included the FIXME I asked. This is not a trivial
matter, even though it's a trivial patch.

The new behaviour is that functions that do not have .cpu directives
in it will start having it. Consider the case where I'm compiling to
ARMv4, but have inline asm on one function that has ".cpu cortex-a15"
to enable hardware divide, because "you know" that only in that case
that function will be executed.

function HardDIV:
  .cpu cortex-a15
  hdiv whatever
  bx lr
function MiscompiledSoftDIV:
  hdiv somethingelse
  bx lr

Before your patch, only compiling the miscompiled function, the
assembler would have found the error. Compiling both functions, you'd
get the error you're trying to fix.

After your patch, only compiling the miscompiled function, the
assembler would have found the error. Compiling both functions, you'll
get no compiler error at all.

This means that, by adding a new cpu-specific function to your
library, you lose the ability to detect miscompilations in the
*entire* library.

Your patch is not testing for any of that, but it should. And it
should include FIXMEs, too.


cheers,
--renato



More information about the llvm-commits mailing list