Provide a consistent interface for asking if trampolines are required #3479

0xdaryl · 2019-01-20T01:02:27Z

Add CPU-specific tests for relative branch reachability
Remove unused AlwaysUseTrampoline CodeGenerator option
Add directCallRequiresTrampoline CodeGenerator query
Use directCallRequiresTrampoline CodeGenerator query

0xdaryl · 2019-01-20T01:04:21Z

This is a WIP while further testing completes.

In the meantime, I'd appreciate a review from @fjeremic @ymanton @0dvictor @knn-k for the platform-specific changes please.

0xdaryl · 2019-01-20T01:43:25Z

@genie-omr build all

0xdaryl · 2019-01-20T04:17:53Z

@genie-omr build aarch64

0xdaryl · 2019-01-20T04:27:25Z

@genie-omr build all

fjeremic · 2019-01-21T15:02:22Z

compiler/aarch64/env/OMRCPU.cpp

+   {
+   intptrj_t range = targetAddress - sourceAddress;
+   return (range <= self()->maxUnconditionalBranchImmediateForwardOffset() &&
+           range >= self()->maxUnconditionalBranchImmediateBackwardOffset()) ? true : false;


Do we need the ternary operator here? Omitting ? true : false accomplishes the exact same thing.

Similarly elsewhere in this PR.

Agreed it is not necessary. I'll fix up everywhere.

fjeremic · 2019-01-21T15:04:45Z

compiler/codegen/OMRCodeGenerator.hpp

+    */
+   bool directCallRequiresTrampoline(intptrj_t targetAddress, intptrj_t sourceAddress)
+      {
+      TR_ASSERT(0, "An architecture specialization of this function must be provided.");


Sounds like a fatal assert. Can we make it as such?

Yes, I'm ok with that suggestion. However, the idea behind returning true was that it should force the user of this API to do the right thing because the target is not apparently reachable (i.e., its a fail safe). A fatal assert, however, would make it more clear to an implementation that it should be overriding this function.

fjeremic · 2019-01-21T15:06:30Z

compiler/z/codegen/OMRSnippet.cpp

@@ -149,7 +149,8 @@ OMR::Z::Snippet::generatePICBinary(TR::CodeGenerator * cg, uint8_t * cursor, TR:
         }
 #endif

-      TR_ASSERT(CHECK_32BIT_TRAMPOLINE_RANGE(destAddr, cursor),  "Helper Call is not reachable.");
+      TR_ASSERT(TR::Compiler->target.cpu.isTargetWithinBranchRelativeRILRange(destAddr, (intptrj_t)cursor),
+                "Helper Call is not reachable.");


Let's make this a fatal assert. I'd rather have a compile time assert failure than unpredictable runtime crash.

Similarly elsewhere in Z codegen where asserts were touched.

ymanton · 2019-01-21T16:28:22Z

compiler/aarch64/codegen/OMRCodeGenerator.hpp

+    *
+    * @return : true if a trampoline is required; false otherwise.
+    */
+   bool directCallRequiresTrampoline(intptrj_t targetAddress, intptrj_t sourceAddress);


How do you feel about making the sourceAddress parameter not be ISA-specific? Rather than having the caller be responsible for knowing what the relative base is and having +4s in some places and not in others might we be better off just passing the call address and target address and letting either the code generator or OMRCPU figure it out? The only down side I can think of is that if the base depends on the type of instruction being used we'd have to also pass that in, but I can't imagine any ISA doing that sort of thing. The upside is that it's just plain easier to understand and use and consistent across code generators.

It does make sense to have a parameter with a clearly defined definition. The CodeGenerator implementation on each platform should be able to determine what adjustments are needed. In theory, all platforms except x86 should be passing in the instruction address already based on my understanding of how the displacements are calculated for their respective branch instructions. X86 will be require a bit of rework that may reach into downstream projects. I'll look into this deeper and report back.

I've made changes to the PR where the sourceAddress of the directCallRequiresTrampoline is the address of the call instruction. For the most part, no changes were required except on x86, and even in that case the changes were minimal. I'll push those changes shortly.

@fjeremic : in the process of doing this I discovered a couple of places in the Z codegen where I believe the wrong source address is being used already to calculate whether a trampoline is needed. Here are the two places that I found in the current code base:

https://github.com/eclipse/omr/blob/7a0ff628cb5c514ebaa88c1bb3d884fd8b4ca6fb/compiler/z/codegen/OMRSnippet.cpp#L142

https://github.com/eclipse/omr/blob/7a0ff628cb5c514ebaa88c1bb3d884fd8b4ca6fb/compiler/z/codegen/S390HelperCallSnippet.cpp#L113

In both cases, I believe cursor is two bytes off because it has already been advanced once the instruction opcode was written. If you confirm this is an issue then I'll fix up as part of this work.

I should point out that in both of the Z cases above the cursor was correctly adjusted during the actual encoding of the instruction (it was just the trampoline test where it is different).

0dvictor · 2019-01-22T20:23:57Z

compiler/arm/codegen/ARMBinaryEncoding.cpp

-             "CodeCache is more than 32MB.\n");
+
+      TR_ASSERT(TR::Compiler->target.cpu.isTargetWithinBranchImmediateRange(target, (intptrj_t)cursor),
+                "Target address is out of range.  CodeCache is more than 32MB.");


It seems the asserting condition does not have any indication that CodeCache must not be more than 32MB. It may be more clear removing "CodeCache is more than 32MB."

knn-k · 2019-01-24T01:55:36Z

For AArch64, TR::ARM64ImmSymInstruction::generateBinaryEncoding() and TR::ARM64LabelInstruction::generateBinaryEncoding() in ARM64BinaryEncoding.cpp can be rewritten using directCallRequiresTrampoline().

To prevent downstream project breakages, introduce empty extensions to the CPU hierarchy for ARM and AArch64. Once merged, downstream projects can make changes to their build systems to accommodate the new files before new CPU functionality is added in eclipse-omr#3479. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

Add tests to each CPU class to answer whether the distance between a source address and a target address falls within the range of a relative, immediate branch instruction on the given CPU type. This is useful, for example, to determine whether a trampoline is required to reach a target method address or helper function directly. These functions deprecate the platform-specific macros and functions currently in use. They will be cleaned up once downstream projects have been modified. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

The option has the same functionality as the StressTrampolines option. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

Add a CodeGenerator query to answer whether a trampoline is required to branch from some source address to a target address. Each platform provides its own specialized implementation of this function. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

* Use the standard query on each platform to determine the need for trampolines for direct calls and helpers. * Use platform-specific CPU queries to test call or branch instrution ranges. Existing macros will be left in place until downstream projects are converted to use the CPU queries. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

0xdaryl · 2019-01-30T20:27:58Z

@genie-omr build all

0xdaryl · 2019-01-30T21:35:04Z

I'm running some final, private stress tests now. All look good so far. I'll un-WIP this when they're done so hopefully this can be merged then (merge conflicts are killing me!)

0xdaryl · 2019-01-30T23:37:02Z

Un-WIPing as my internal stress testing is clean. I believe I've addressed all the comments.

…ange This is a follow-up of eclipse-omr#3479 for aarch64. Two functions in ARM64BinaryEncoding.cpp to use cg->directCallRequiresTrampoline() for checking the range of branch instructions. Signed-off-by: knn-k <konno@jp.ibm.com>

0xdaryl added the comp:compiler label Jan 20, 2019

0xdaryl requested review from fjeremic and mstoodle as code owners January 20, 2019 01:02

0xdaryl force-pushed the cpubranchdisp branch 2 times, most recently from bd6ec27 to 49e13ad Compare January 20, 2019 01:42

0xdaryl requested review from charliegracie and Leonardo2718 as code owners January 20, 2019 01:42

0xdaryl force-pushed the cpubranchdisp branch 2 times, most recently from 84ce1bd to 168b3a5 Compare January 20, 2019 04:16

fjeremic reviewed Jan 21, 2019

View reviewed changes

ymanton reviewed Jan 21, 2019

View reviewed changes

0dvictor reviewed Jan 22, 2019

View reviewed changes

0xdaryl force-pushed the cpubranchdisp branch 3 times, most recently from 3546280 to ebf40ba Compare January 24, 2019 15:07

0xdaryl mentioned this pull request Jan 24, 2019

Introduce empty CPU extension classes for ARM and AArch64 #3510

Merged

0xdaryl force-pushed the cpubranchdisp branch 2 times, most recently from e19e295 to 30e44d7 Compare January 29, 2019 00:09

0xdaryl added 4 commits January 30, 2019 14:24

Remove unused AlwaysUseTrampoline CodeGenerator option

02f006e

The option has the same functionality as the StressTrampolines option. Signed-off-by: Daryl Maier <maier@ca.ibm.com>

0xdaryl force-pushed the cpubranchdisp branch from 30e44d7 to 7fb31ec Compare January 30, 2019 19:31

fjeremic approved these changes Jan 30, 2019

View reviewed changes

0dvictor approved these changes Jan 30, 2019

View reviewed changes

fjeremic self-assigned this Jan 30, 2019

0xdaryl changed the title ~~WIP: Provide a consistent interface for asking if trampolines are required~~ Provide a consistent interface for asking if trampolines are required Jan 30, 2019

fjeremic merged commit de6dd94 into eclipse-omr:master Jan 31, 2019

knn-k mentioned this pull request Feb 20, 2019

AArch64: Use cg->directCallRequiresTrampoline() for checking branch range #3586

Merged

0xdaryl mentioned this pull request Feb 21, 2019

Add Machine class queries for conditional/unconditional branch displacements #2962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a consistent interface for asking if trampolines are required #3479

Provide a consistent interface for asking if trampolines are required #3479

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

fjeremic Jan 21, 2019

fjeremic Jan 21, 2019

0xdaryl Jan 21, 2019

fjeremic Jan 21, 2019

0xdaryl Jan 21, 2019

fjeremic Jan 21, 2019

fjeremic Jan 21, 2019

0xdaryl Jan 21, 2019

ymanton Jan 21, 2019

0xdaryl Jan 21, 2019

0xdaryl Jan 22, 2019

0xdaryl Jan 22, 2019

0dvictor Jan 22, 2019

0xdaryl Jan 22, 2019

knn-k commented Jan 24, 2019 •

edited

Loading

0xdaryl commented Jan 30, 2019

0xdaryl commented Jan 30, 2019

0xdaryl commented Jan 30, 2019

Provide a consistent interface for asking if trampolines are required #3479

Provide a consistent interface for asking if trampolines are required #3479

Conversation

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

0xdaryl commented Jan 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knn-k commented Jan 24, 2019 • edited Loading

0xdaryl commented Jan 30, 2019

0xdaryl commented Jan 30, 2019

0xdaryl commented Jan 30, 2019

knn-k commented Jan 24, 2019 •

edited

Loading