I'm writing a set of lirasm tests for another bug, and some of the tests I want to add use LIR instructions that apply only to soft-float configurations. The attached patch adds a "tests/softfloat" directory and some platform-detection code. I'm using this for local testing but it makes sense to share it as it should be generally useful, particularly as it also adds a framework for testing other things, such as 64-bit-specific instructions. One possible issue is that it overlaps with bug 530754. The patch over there was accepted but for some reason never pushed. In fact, I had forgotten about it until I'd finished writing this one, else I would have used it as a baseline for this one. Comments welcome.

If we're only going to be testing ARM6/ARM7 with VFP on, and ARM5 with VFP off, perhaps we should add compiletime or runtime warnings, indicating these are unsupported (or at least untested) combinations.

Comment on attachment 453384 [details] [diff] [review] Add soft-float tests and automate testing of alternate configurations for ARM. >-for infile in "$TESTS_DIR"/*.in >-do >+function runtest { >+ infile=$1 >+ options=${2-} Do you know about bash's "local" keyword? It's good for local variables, avoids leaving behind globally visible env vars. >+# Tests common to all supported back-ends. >+for infile in "$TESTS_DIR"/*.in >+do >+ runtest $infile > done > >+# ---- Platform-specific tests and configurations. ---- >+ >+# ARM >+if [[ $(uname -m) == arm* ]] >+then >+ for infile in "$TESTS_DIR"/*.in >+ do AFAICT you're running the all the vanilla tests ("Tests common..."), then if it's ARM you re-run most of them. Am I missing something? >+ # Skip the random tests, simply because they take too long to run. >+ if [[ `basename $infile` = "random.in" || `basename $infile` = "random-opt.in" ]] >+ then >+ continue >+ fi Please don't skip the random tests! They've found more bugs in the ARM backend than any other. Please instead change the script so that they run with a smaller fragment size. The size of 1000000 is hardwired into the script above, maybe you could add another argument to 'runtest' which is the random fragment size. This script assumes that you have a V7 or higher machine, right? It would be nice if it didn't assume that (my VM is V6 or V5, I can't remember which). Maybe you could do a little more feature detection to cover this? But it's not the end of the world if you can't. This is much better than what I had in bug 530754. I never landed that patch because (a) it was a bit hacky, and (b) my interest in testing SoftFloat plummeted when I learnt that no Mozilla products use it (see bug 543636).

(In reply to comment #2) > Do you know about bash's "local" keyword? It's good for local variables, > avoids leaving behind globally visible env vars. I do now! (My Bash knowledge is terrible.) > AFAICT you're running the all the vanilla tests ("Tests common..."), then if > it's ARM you re-run most of them. Am I missing something? If you pass no --arch flag to lirasm, it defaults to ARMv7, so it re-runs most of the tests for other combinations of --arch and --[no]vfp. > Please don't skip the random tests! They've found more bugs in the ARM backend > than any other. Please instead change the script so that they run with a > smaller fragment size. The size of 1000000 is hardwired into the script above, > maybe you could add another argument to 'runtest' which is the random fragment > size. It still runs the random tests for ARMv7, but skips them for ARMv6 etc. Adding them back in with a smaller size is feasible. Actually, there's probably no harm in having them run the full test. > This script assumes that you have a V7 or higher machine, right? It would be > nice if it didn't assume that (my VM is V6 or V5, I can't remember which). > Maybe you could do a little more feature detection to cover this? But it's not > the end of the world if you can't. It does assume that, but so does lirasm itself unless you override --arch. (We didn't do this before.) I'm surprised it worked on your ARMv6 emulator as NativeARM.cpp will make heavy use of MOVW and MOVT, which aren't supported by most of ARMv6. I should be able to add better feature detection. `uname -m` is pretty easy to parse for ARM. > This is much better than what I had in bug 530754. I never landed that patch > because (a) it was a bit hacky, and (b) my interest in testing SoftFloat > plummeted when I learnt that no Mozilla products use it (see bug 543636). Yeah, the soft-float stuff is a nuisance really, but I have to change the code for bug 535709 and it makes sense to add tests whilst I'm at it. Adobe still care about soft-float. (In reply to comment #1) > If we're only going to be testing ARM6/ARM7 with VFP on, and ARM5 with VFP off, > perhaps we should add compiletime or runtime warnings, indicating these are > unsupported (or at least untested) combinations. That might be a good idea. Is that an acceptable set of configurations for Adobe? I'd like to remove ARMv5 if possible but I think you still require it.

Addresses most of the comments: * Improved handling of random tests. Older targets still run them, but with a reduced size. * Uses local variables in the Bash script. * I added the warning about untested configurations to lirasm.cpp. It won't show up in TM or TR, but this is easily changed if you prefer it. As discussed on #nanojit, I didn't incorporate the platform-detection stuff as it's not really what this bug is about (and the patch doesn't make matters worse). If it's important, we should open a new bug to handle it. I think this is pretty much ready, but I want to check that the list of targets to be tested is good for Adobe before I push it.

(In reply to comment #3) > Adobe still care about soft-float. Proabably more accurate to say that we aren't yet convinced we can give up on it -- I'd dearly love to flush it and require VFP, but we're concerned that some hardware partner will insist on a non-VFP device. I'm skeptical that a non-VFP device would provide acceptable performance, but... > That might be a good idea. Is that an acceptable set of configurations for > Adobe? I'd like to remove ARMv5 if possible but I think you still require it. Hmm, good question. I think there might be some emulator configurations that are ARMv5 only. What's the motivation for dropping ARMv5... does it simplify testing or code significantly?

(In reply to comment #4) > I think this is pretty much ready, but I want to check that the list of targets > to be tested is good for Adobe before I push it. I've pinged the relevant folks at Adobe -- it might be a day or two before I hear back.

(In reply to comment #5) > > That might be a good idea. Is that an acceptable set of configurations for > > Adobe? I'd like to remove ARMv5 if possible but I think you still require it. > > Hmm, good question. I think there might be some emulator configurations that > are ARMv5 only. What's the motivation for dropping ARMv5... does it simplify > testing or code significantly? Hmm. I _would_ like to remove it entirely, but that is a question for a wider audience and I don't think we should try to resolve it in this bug. I was talking about removing ARMv5 and soft-float support only from the lirasm test run. However, that leaves a fairly large portion of code that isn't tested*, and that makes me uncomfortable. I think I'd rather leave it in there for now, or perhaps remove it entirely as part of another bug. * I know you have automated tests for soft-float builds of Tamarin, but by that point it's already in nanojit-central and probably tracemonkey too, and it seems safer to leave the tests in lirasm if possible. (In reply to comment #6) > I've pinged the relevant folks at Adobe -- it might be a day or two before I > hear back. Over in bug 543636, a few Adobe people concluded that it was still important, and that's why it still exists.

(In reply to comment #7) > Hmm. I _would_ like to remove it entirely, but that is a question for a wider > audience and I don't think we should try to resolve it in this bug. Oliver Goldman on the Adobe AIR team informs me: "Supporting ARMv5 is required: AIR Android supports the Android SDK simulator, which is ARMv5 only. (We build two versions of AIR Android: ARMv5 for the simulator, and ARMv7 for real devices.) Mind you, I'd rather solve this problem the other way, by having someone update the simulator." > I was talking about removing ARMv5 and soft-float support only from the lirasm > test run. However, that leaves a fairly large portion of code that isn't > tested*, and that makes me uncomfortable. Agreed.

Comment on attachment 453384 [details] [diff] [review] Add soft-float tests and automate testing of alternate configurations for ARM. Adding a posthumous r+ to clean out my review queue.