702278 - JaegerMonkey on ARM doesn't respect assumed repatching rules.

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Jacob Bramley [:jbramley]]

When branches are emitted on ARM, the back-end takes a special flag. The flag is currently named 'useConstantPool', though this is technically incorrect. What is means is 'this branch (or load) needs to remain patchable even after code finalization'. If the flag is clear, we might try to share a literal pool slot, or optimize the branch in some way.

In at least once instance, in the call compiler, we call masm's jump() method, which generates a non-repatchable branch, but then repatch it. I don't think this error ever actually occurs in current code, but it was highlighted by my LDR-to-B patch (bug 586297), which aggressively optimizes branches and suchlike. (If it can occur in current code, it would cause an even rarer instance of the bug.)

It is trivial to make jump() default to useConstantPool=1, but this might not be the best fix: If there is only one instance of jump() that needs repatching, a more efficient solution would be to edit that instance. However, I did test that under my LDR-to-B patch and everything seems to be working. I have not yet assessed any performance hit.

Comment 1

[Jacob Bramley [:jbramley]]

Ok, there is no existing bug: Although we track the useConstantPool flag, possibly incorrectly, we don't actually read it. WebKit used to, but I disabled that code when JM was first integrated (for reasons explained in the code).

Forcing useConstantPool=1 everywhere seems to have a significant effect on the optimization in bug 586287, to the point that it's barely worthwhile. It still goes faster on average, but not so much that the complexity is justified.

The problem really is that the API doesn't have sufficient meta-information about the branches. For example, we don't indicate local branches, and we don't give the assembler information about whether the branch could be patched or not. These things could be added, but it might take quite a bit of work.

Another option is to modify the way that we concatenate code. We know the code size of the fast and slow paths, so we could patch the branches before concatenating. We could then optimize the local branches (that will never be repatched) before finalizing the code. However, we still can't reliably tell whether a branch is repatchable or not.

(By "repatchable" I mean "can be patched after ARMAssembler::fixUpOffsets".)

Comment 2

[Jacob Bramley [:jbramley]]

This isn't actually a bug in the existing code. Fixing it might enable further optimizations, but preliminary tests didn't show encouraging results. Some benefit could probably be gained with some work, but Ion Monkey handles local branches more efficiently anyway.