search

parallaxinc / OpenSpin

Spin/PASM compiler for the Parallax Propeller.
56 stars 19 forks source link

Unused Method Cleanup #21

Closed bweir closed 6 years ago

bweir commented 9 years ago

Hi Roy,

I've been away from all aspects of Propeller development for awhile (long story), otherwise I would have told you this months ago. Nevertheless, unused method cleanup still isn't stable enough for me to release as part of PropellerIDE.

Now that I've returned to work, my first order of business is automating my release processes. As part of this, I have set up a continuous integration server for my distribution of OpenSpin, which will now run regression testing of the compiler against the Spin library distribution as well as the LameStation SDK, and hopefully more software packages later.

Anyhow, I'm not sure how these were introduced, but it seems like there are a lot more errors from the previous release. I'm not even yet building from my Qt version. I'm pulling straight from upstream for these tests, and I have confirmed locally that building against 1.00.77 has similar results.

See for yourself here. https://travis-ci.org/bweir/OpenSpin/jobs/82146770

From now on, when you have updates, it'll be possible to test them immediately. You can get results even faster by incorporating continuous integration into the mainline.

Anyway, I thought you should know. Are you still working on these issues or any other aspect of OpenSpin?

Thanks,

PropGit commented 9 years ago

Thanks Brett. Roy and Parallax talked a while ago about OpenSpin, but we were not aware of this.

reltham commented 9 years ago

I never got an email about this issue. Not sure why, I get emails for other stuff from github.

Anyway, I had successfully compiled all your stuff (you had pointed me at a zip to test with). Not sure why you are now getting errors. Are you certain you are testing against the proper code/build of openspin.exe?

In any case, where can I easily get a zip of all your latest spin stuff so i can test against it? Your link above doesn't tell me much.

bweir commented 9 years ago

Who knows. I get weird email behavior sometimes too.

Yes, I'm not sure what happened either. This log shows a build from my modified tree, but I also built against your latest release version with the same results. I am also building with both clang and g++ and having the same results.

Here is the latest SDK release link: https://github.com/lamestation/lamestation-sdk/releases/download/rc2-0.3.2/lamestation-sdk-rc2-0.3.2.zip

Thanks for looking into it.

On Sun, Oct 11, 2015 at 5:00 AM, Roy Eltham notifications@github.com wrote:

I never got an email about for this issue. Not sure why, I get emails for other stuff from github.

Anyway, I had successfully compiled all your stuff (you had pointed me at a zip to test with). Not sure why you are now getting errors. Are you certain you are testing against the proper code/build of openspin.exe?

In any case, where can I easily get a zip of all your latest spin stuff so i can test against it? Your link above doesn't tell me much.

— Reply to this email directly or view it on GitHub https://github.com/parallaxinc/OpenSpin/issues/21#issuecomment-147186236 .

Brett Weir, Founder

Engineering Made Awesome 310.245.1775 • brett@lamestation.com • lamestation.com

reltham commented 9 years ago

Brett, I have submitted a fix that should resolve your errors. I tested with several of your objects that were failing and they now succeed. Can you check it with your whole build test and let me know so I can close this issue, or fix anything else you find? Thanks!

bweir commented 9 years ago

Hi Roy, Thanks for the update! I tried it again and the error count has gone down from 47 to 3. Still a couple left, but it's getting close!

../build/openspin -u -L lamestation-sdk-rc2-0.3.2 -L . ./lamestation-sdk-rc2-0.3.2/games/piXel/piXel_Effects.spin
Propeller Spin/PASM Compiler 'OpenSpin' (c)2012-2015 Parallax Inc. DBA Parallax Semiconductor.
Version 1.00.77 Compiled on Nov  9 2015 22:09:02
Compiling...
./lamestation-sdk-rc2-0.3.2/games/piXel/piXel_Effects.spin
|-LameGFX.spin
|-piXel_Sound.spin
  |-LameAudio.spin
    |-LamePinout.spin
  |-LameFunctions.spin
|-gfx_boom.spin
Done.
|-piXel_Sound.spin
piXel_Sound.spin(90:1) : error : No PUB routines found
Line:
End Of File
Offending Item: N/A

../build/openspin -u -L lamestation-sdk-rc2-0.3.2 -L . ./demos/SingingDemo.spin
Propeller Spin/PASM Compiler 'OpenSpin' (c)2012-2015 Parallax Inc. DBA Parallax Semiconductor.
Version 1.00.77 Compiled on Nov  9 2015 22:09:02
Compiling...
./demos/SingingDemo.spin
|-VocalTract.spin
|-StereoSpatializer.spin
Done.
|-VocalTract.spin
./demos/SingingDemo.spin(73:10) : error : Expected a subroutine name
Line:
  stereo.start(@input, @buffer, buffer_size, 11, -1, 10, -1) 'start spatializer
Offending Item: start

../build/openspin -u -L lamestation-sdk-rc2-0.3.2 -L . ./demos/SingingDemoSeven.spin
Propeller Spin/PASM Compiler 'OpenSpin' (c)2012-2015 Parallax Inc. DBA Parallax Semiconductor.
Version 1.00.77 Compiled on Nov  9 2015 22:09:02
Compiling...
./demos/SingingDemoSeven.spin
|-VocalTract.spin
|-StereoSpatializer.spin
Done.
|-VocalTract.spin
./demos/SingingDemoSeven.spin(72:10) : error : Expected a subroutine name
Line:
  stereo.start(@input, @buffer, buffer_size, 11, -1, 10, -1) 'start spatializer
Offending Item: start
reltham commented 9 years ago

Looking at piXel_Effects.spin, it's not really meant to be compiled as a root object. It doesn't actually call any of the functions anywhere else from it's first PUB. So unused method elimination will remove everything except for the methods referred to in COGINIT. It's kind of an invalid test case.

Not really sure if it's really worth spending the time to fix this case, since it's not a valid spin root object that would do anything useful. It works fine when compiling the whole piXel game.

The other two used the rare array of objects feature of spin that I broke with this fix, but I have just submitted a fix for that. This also bumps the version to 1.0.78. Can you try this new version and verify it fixes the SingingDemo ones.

reltham commented 9 years ago

Just to clarify what is happening with piXel_Effects.spin.

It uses piXel_Sound.spin which has a Start PUB that does a cognew that refers to a PRI method. The Unused Method stuff forces any PUB or PRI referred to by a cognew or coginit to be included. However, in this case, the code never calls any of the PUB methods in piXel_Sound.spin and you are left with just the PRI methods. That is not valid spin code, so you get the error.

Due to the way coginit/cognew stuff works, it's not possible to know what method issued the coginit/cognew. It's PUB/PRI index isn't known at the time it's actually making the bytecode for it (it's known a little later). So my tracking code can't check to see if the function that has the cognew/coginit in it is never called to exclude it also.

I could spend some time trying to make this case not cause an error, but is it really worth it for a case that isn't meant to actually be valid anyway? Jeff would have to approve doing the extra work.

Again, in the proper context where this code is used from someplace that actually calls the Start method in piXel_Sound.spin, it will compile successfully with unused method removal on.

PropGit commented 8 years ago

@reltham - I missed this until our recent conversation. Thanks for the very clear explanation of the problem. I've been thinking about this today and believe I see the whole picture now and what the solution is.

Confirmation

This is definitely a case we hadn't thought about before.

You're right, and I agree, that the way piXel_Effects (and it's subobject piXel_Sound) is being used in this particular case (as a top file) doesn't make sense by convention or by its true big-picture intent- piXel_Effects is meant to be a subobject itself. However, I can envision cases where this is desired, maybe temporarily and maybe permanently.

Deciding Point

The application in this case (when compiled with piXel_Effects as the top object and with no optimization) compiles successfully. Though it includes plenty of unused code and wasted space when compiled that way, it is syntactically valid and generates properly-executing code. For this reason, we need to make sure that optimized compilations are successful in generating equally executing code (with less wasted space). That is, after all, the exact goal of our optimization feature - to remove both intentionally and unintentionally unused code alike, while keeping the actual executable code in-tact.

As it happens, this case is similar to a possibility I've previous thought about taking advantage of: being able to create an object that has two uses/behaviors; one as a subobject and one as a top object. In one possible case, as a subobject, it may only logically serve as a source for defined constants, with no referenced executable code. To make this possible even with optimization enabled, the compiler needs to retain an object's constant values for possible reference by a parent object (I think OpenSpin is already doing this) and yet mark the entire object as discarded if it finds that there are no public methods referenced.

Solution

For the Spin language, going forward, there's good reason to relax the must-contain-at-least-one-PUB rule so that it only applies to top objects and not to subobjects. This would allow an object to fit the dual-behavior pattern I stated above, and also to be used simply as a common source of constants (for the entire application object hierarchy) and even for a source of single-instance data and PASM code if we further enhance Spin to support this.

So, can you make it allow PUB-less objects for all subobjects, regardless of whether or not -u is enabled?

And, can/will it follow this logic, below? Note: struck out items may be implemented later after further consideration.

NOTE: For the case where the PUB-less subobject is included in-full (no optimization), I've tried to verify (by manually editing a binary) that the Spin interpreter continues to work properly. It works if I adjust the method count and clear out the address reference and the code (from that object's jump table in the image) but if I move the following object header and code, it breaks (fails to run properly). Perhaps I'm forgetting to adjust something.

reltham commented 6 years ago

So I made a small change here that should resolve the specific case for piXel_Effects.spin being compiled as a top object (and any other similar cases).

With UnusedMethodElimination, if an object has no methods called, then the cognew/coginit referenced methods will not be marked as called since the methods containing the coginit/cognew are not called. This will cause the object to be eliminated (as expected).

The general case of allowing objects to contain no PUB's is a more involved change that I don't think is worth the effort. You can just put in a PUB and never call it and it will work out the same. If that object is compiled as a top object then the first PUB will be force included (when using -u) and when you compile it as a sub object it will be eliminated but still allow CON stuff to be used in the parent.