Closed Bug 526974 Opened 15 years ago Closed 13 years ago

lirasm: Add configuration options to lirasm test utility.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jbramley, Assigned: jbramley)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

For example, I want to be able to test the ARMv5 code paths where no ARMv5 hardware is available _without_ having to rebuild or hack in some magic. This isn't as good as actually running on an ARMv5 target, but it's a decent half-way stage!

I have a patch (which I will upload shortly) that adds the "--arm-arch" and "--arm-[no]vfp" options to lirasm. (They still default to ARMv7+VFP; there's no dynamic feature detection in lirasm.) It also allows you to configure these via "LIRASMFLAGS" when you do "make check".

I'm not sure if this fits neatly with the rest of the lirasm plans, but this is what I've been using locally for the development of an unrelated patch, and it makes sense to push it into the trunk as it seems generally useful.
This applies to the lirasm in nanojit-central.
Attachment #410749 - Flags: review?(nnethercote)
Attachment #410749 - Flags: review?(nnethercote) → review-
Comment on attachment 410749 [details] [diff] [review]
Adds "--arm-arch", "--arm-[no]vfp" and LIRASMFLAGS.

>+LIRASMFLAGS ?=

I like the idea of allowing options to be specified via an environment variable, but the way you've done it only works if you're running "make check".  I'd prefer this env var to be read within lirasm itself and so always work.  I'd also like this env var to be mentioned in the --help message.

>+#if defined NANOJIT_ARM
>+        // ARM configuration options.

I see two ways to handle arch-specific options.  One is to always parse the option and show it in the --help message, but complain if it's actually given on a non-relevant platform;  that's what I did for --sse.  The other is to conditionally compile all the arch-specific code, which is what you've done.  We should be consistent.  I'll argue for my approach because (a) it reduces the amount of conditionally-compiled code, and (b) it's what's already there :)

Also, I have strtol() checking for --random, you might as well copy it (or factor it out) for --arm-arch.
(In reply to comment #2)
> (From update of attachment 410749 [details] [diff] [review])
> >+LIRASMFLAGS ?=
> 
> I like the idea of allowing options to be specified via an environment
> variable, but the way you've done it only works if you're running "make check".
>  I'd prefer this env var to be read within lirasm itself and so always work. 
> I'd also like this env var to be mentioned in the --help message.

Hmm. In general, I don't like using environment variables for passing parameters to programs as they get a bit messy. However, I can see the value here. I prefer the command-line "--arm-arch" method as it's much easier to reproduce and you'll get explicit errors if you spell it wrong, whereas an environment variable just gets ignored.

The LIRASMFLAGS variable in the Makefile exists purely because it's the only practical way of passing parameters into 'make' without first running configure, which seems over-the-top for a flag that just sets AvmCore::config differently. I think of it as an equivalent to "CCFLAGS".

It _should_ be documented somewhere, and I apologize for that, but I'm not sure where to put it. Perhaps the nanojit-central wiki page is the best place, but there's no convention for "make --help" so adding it there doesn't help much.

> >+#if defined NANOJIT_ARM
> >+        // ARM configuration options.
> 
> I see two ways to handle arch-specific options.  One is to always parse the
> option and show it in the --help message, but complain if it's actually given
> on a non-relevant platform;  that's what I did for --sse.  The other is to
> conditionally compile all the arch-specific code, which is what you've done. 
> We should be consistent.  I'll argue for my approach because (a) it reduces the
> amount of conditionally-compiled code, and (b) it's what's already there :)

Yes, your approach is better.

> Also, I have strtol() checking for --random, you might as well copy it (or
> factor it out) for --arm-arch.

Good point! That was mostly laziness on my part.
I'd vote for command-line flags rather than environment variables also. Assuming it's a democracy :)
(In reply to comment #3)
>
> I prefer the command-line "--arm-arch" method as it's much easier to
> reproduce and you'll get explicit errors if you spell it wrong, whereas an
> environment variable just gets ignored.

I was imagining that the env var would effectively get tacked onto the end of argv[] so that any incorrect options would cause an abort.
 
I also don't like env vars in general, but it seems like an acceptable use of them to get around the fact that it's hard to pass options to "make check".  If you want to keep the exact mechanism from the patch, I'd be ok with it so long as the env var's name was less misleading, eg. FLAGS_PASSED_TO_LIRASM_WHEN_MAKE_CHECK_IS_INVOKED or something like that... LIRASMFLAGS makes it sounds like a general purpose thing (and my original suggestion would have made it into a general purpose thing).

But this leads to a broader question:  this only helps us if we remember to use the env var.  Should we automate this?  Eg. for each arch, have multiple configurations that we run all the tests on when we do "make check"?  On x86 we have SSE and non-SSE, on ARM we have multiple combinations.
Yes, we should automate it and run as many variants as we can. Should also run larger blocks of --random, +/- with fixed and random seeds, and under valgrind when possible. Many upgrades to the makefile to come, separate patches :)
Yes, but the variants idea is directly relevant to whether or not LIRASMFLAGS should be added.  Perhaps this bug should be renamed "test multiple arch variants" or somesuch.
(In reply to comment #5)
> (In reply to comment #3)
> >
> > I prefer the command-line "--arm-arch" method as it's much easier to
> > reproduce and you'll get explicit errors if you spell it wrong, whereas an
> > environment variable just gets ignored.
> 
> I was imagining that the env var would effectively get tacked onto the end of
> argv[] so that any incorrect options would cause an abort.

End, or beginning? In order of variability or override priority, it seems envar is trumped by argv. That's what I've used in past programs that had both ways of specifying options.

Making the envar take a list of options, just as they would appear on the cmd line, takes away the stink of having two ways to do it, IMHO.

/be
If I understand correctly, the argument has come back to what I did originally, with argv overriding the environment and lirasm itself ignoring the environment. Is this correct?

There are a few other changes to make, based on Nicholas's review, but leave the configuration mechanism as it is.

Automating different configurations is a fantastic idea but should go in a different bug.
Summary: lirasm: Add ARM configuration options to lirasm. → lirasm: Add configuration options to lirasm test utility.
(In reply to comment #9)
> If I understand correctly, the argument has come back to what I did originally,
> with argv overriding the environment and lirasm itself ignoring the
> environment. Is this correct?

Yes, modulo the change in name of the env var.

> There are a few other changes to make, based on Nicholas's review, but leave
> the configuration mechanism as it is.
> 
> Automating different configurations is a fantastic idea but should go in a
> different bug.

The infrastructure for that may end up getting rid of the env var, but I'm happy for it to go in a different bug just so you can get this step working.  Can you file the follow-up bug?
Ok, this incorporates your feedback:

 - The environment variable is now called "MAKE_CHECK_LIRASM_FLAGS", but works in the same way as before.
 - I've filled in the usage information. I'm not sure how it didn't make it into my original patch!
 - I've renamed the command line options to remove the "arm-" prefix, to make them consistent with the "--sse" options. We could add "i386-" to make "--i386-sse", but we'll get long-winded so I opted to simply remove the prefix; there will never be a naming conflict as these are platform-specific, but if you prefer to have the prefixes then feel free to override my decision.
 - Similarly, I've _added_ a "i386_" prefix to the "sse" configuration variable in the code to make it consistent with the "arm_" options. This seemed logical here, but again, feel free to override.
 - I've kept the #ifdef blocks because it was the neatest way I could find to write the code. However, they are tidier than they were in my original patch, and i386 & ARM options are both handled in the same way now.

The patch passes all the tests on ARM and Intel, and seems to work Ok in terms of selecting the appropriate configuration.
Attachment #410749 - Attachment is obsolete: true
Attachment #414220 - Flags: review?
Attachment #414220 - Flags: review? → review?(nnethercote)
Blocks: 530754
> Can you file the follow-up bug?

Yep; see bug 530754.
Attachment #414220 - Flags: review?(nnethercote) → review+
http://hg.mozilla.org/tracemonkey/rev/7ca62ed84eb5
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/7ca62ed84eb5
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: