[PATCH]: finish .cpu support for ARM

Renato Golin renato.golin at linaro.org
Wed Dec 3 13:26:04 PST 2014


On 3 December 2014 at 19:28, Roman Divacky <rdivacky at vlakno.cz> wrote:
> What do you mean it doesnt? The patch uses ComputeAvailableFeatures()
> and setAvailableFeatures() which are used in many other places.

Sorry, I meant there isn't a comprehensive test for cpu name
validation. Setting the features is the other problem.


> Also, I really dont see how testing _all_ cpus will help testing
> those two functions.

That was a tangent that we followed too deeply. I said there wasn't a
comprehensive test for CPU names, and we unintentionally got to
validating them all. Ignore that, I agree it's pointless.


> Yes, the patch changes the semantic of assembling files. How does testing
> switching to all variants of cpus change anything about that? Or is this just
> asking for a FIXME?

We're mixing the two ideas again... What I said, and I'm still worried
about, is that you have introduced a seriously bad behaviour without a
single line of testing or warning.

This bad behaviour is "acceptable" for the time being because it's the
lesser of two evils, and it just happens to be what GCC does. Most
people in GCC agree that this is a bad idea, but they won't change
there. We have discussed it here, and most of us agree that it has to
be better in LLVM. But that's for the future. Ignore that, too.

I'd like to have one test with a code similar to what I wrote in my
email, at least for modifying cpus (the hdiv example is a good one),
and a check for errors if the test doesn't warn about the HDIV on the
soft-div implementation. The test will fail, and you should mark it as
XFAIL. If you don't want to add fpu and arch similar examples (NEON
and Thumb are good use cases), I can do that later.

When you commit, let me know the filename, and I'll add to those bugs
that, whomever fix the issue (most likely me), has to remove the XFAIL
and make it pass.

You're probably wondering why not just add those tests when I fix it.
The reason is more educational than technical. I want people that look
at that commit to be wary that it's not a simple bugfix, but it will
incur in a big behaviour change. So when we're doing backporting for
stable releases, we don't pick them up by accident. Similarly, for
people working outside the tree, to be careful with that change.


> I have nothing against adding a FIXME (can you provide the text you want?)

Just a FIXME on the code, explaining in a few lines that this will
potentially disable miscompilation errors in other functions in the
same file that should have that CPU/FPU/ARCH level, with the two bug
numbers I sent you as reference. I wouldn't mind if you added the same
comments where arch and fpu set the bits, too.

Sorry for the confusion, brains don't multi-thread as well as I'd like... :)

cheers,
--renato



More information about the llvm-commits mailing list