This is an alternate implementation of the phi node helper functions proposed in this patch: https://bugzilla.mozilla.org/attachment.cgi?id=468473&action=diff Unlike that version, it does not depend on phi nodes, but simulates them instead with a temporary stack slot. It was necessary to adjust the API slightly, by requiring a call to CodegenLIR::prepareLabelForMerge() in order to allocate the stack temporary early enough. Other than this, the patch is a faithful to the proposed API, and is intended as a stub to allow work on inlining to proceed independently of modifications to NanoJIT, and to facilitate experiments comparing stack temporaries vs. phi nodes.

A few small helper methods, including prerequisites for the previous patch. A small cleanup: In CodegenLIR::loadIns(), I removed an unnecesssary assertion and a cast on the 'disp' argument, which is declared as int32_t in the prototype. I notice that there are call sites where the actual argument is declared 'int'. I wonder whether perhaps the prototype is a mistake, and the assertion and cast should remain. As things stand, we may allow a silent truncation to take place due to the prototype. Or, perhaps the mistake is that the actual argument is not declared 'int32_t'. There is some value, however, it allowing an integer to default to the natural word size on the platform as long as 32 bits is sufficient.

This is just provided as a use case that will build and pass acceptance tests with the patches above. I'm not floating it for review at this time.

(In reply to comment #0) > Created attachment 470131 [details] [diff] [review] > Helper functions for merging values at control-flow joins (alternative > implementation not requiring phi nodes) There is a small amount of duplicated code in determining the store opcode to use in branchToLabelWithValue() and fallToLabelWithValue() which could be pulled out and named, e.g., LOpcode storeOpForLTy(LTy ty). This is deliberate, to make it a bit easier to rip out the temp-based implementation and replace it with a phi-based implementation by keeping the affected methods self-contained.

Remove vestigial code left over from phi-based version.

Comment on attachment 470135 [details] [diff] [review] Simple helper methods, including prerequisites for the value merge patch nit: we probably should be consistent in our declarations. ie. 'LIns* foo()' rather than 'LIns *foo()'

(In reply to comment #1) > A small cleanup: In CodegenLIR::loadIns(), I removed an unnecesssary assertion > and a cast on the 'disp' argument, which is declared as int32_t in the > prototype. I notice that there are call sites where the actual argument is > declared 'int'. I wonder whether perhaps the prototype is a mistake, and the > assertion and cast should remain. As things stand, we may allow a silent > truncation to take place due to the prototype. Or, perhaps the mistake is that > the actual argument is not declared 'int32_t'. There is some value, however, > it allowing an integer to default to the natural word size on the platform as > long as 32 bits is sufficient. int == int32_t, on all platforms we support, and probably ever will support. So, i'm not very worried about this. +1 for consistency, though.

Can you fill in the depends and blocks field on this bug so I can wrap my head around the patch dependencies (or are there really none?)

Pushed to TR: http://hg.mozilla.org/tamarin-redux/rev/24b33feaff16

At present, it does not appear that there is a need for this patch: https://bugzilla.mozilla.org/attachment.cgi?id=470142 Therefore, I am closing the bug on the basis of the smaller patch in comment #9.