Clean up centralized opcode enums

[0xdaryl]

Issue created from this Slack discussion: https://eclipse-omr.slack.com/archives/CBSE78L4U/p1606341331037200?thread_ts=1606338276.029900&cid=CBSE78L4U

Filip Jeremic Nov 25th at 4:04 PM
I’m looking at further cleaning up the now centralized opcode enums following the PRs recently merged. I want to provide a proper way of downstream projects to override tree evaluators. My thoughts on this is to have a simple conceptual model where every opcode is backed by an Evaluator function which makes it trivial to look up an evaluator on each platform. It also makes it possible for downstream projects to override evaluators to insert their own logic because the TR::TreeEvaluator class is extensible. To do this we have to eliminate the list of defines here:
https://github.com/eclipse/omr/blob/1f46e75cf508f47d2d2d8ce48678336f6d845f7f/compiler/x/amd64/codegen/OMRTreeEvaluatorTable.hpp#L27-L737
And introduce a function for each opcode, if one doesn’t already exist. I’ll take the example of TR::BNDCHK:
https://github.com/eclipse/omr/blob/1f46e75cf508f47d2d2d8ce48678336f6d845f7f/compiler/x/amd64/codegen/OMRTreeEvaluatorTable.hpp#L609
We would have to introduce a new function like this:

TR::Register *OMR::X86::AMD64::TreeEvaluator::BNDCHKEvaluator(TR::Node *node, TR::CodeGenerator *cg)
   {
   return TR::TreeEvaluator::badILOpEvaluator(node, cg);
   }

This has the benefit of evaluator names being consistent across platforms. For example if I’m looking for the evaluator for the TR::luneg opcode, I know I will find it as TR::TreeEvaluator::lunegEvaluator on every platform. Instead today we have:
x86 32-bit: TR::TreeEvaluator::integerPairNegEvaluator
x86 64-bit: TR::TreeEvaluator::integerNegEvaluator
Power, ARM, Z: TR::TreeEvaluator::lnegEvaluator
AArch64: TR::TreeEvaluator::unImpOpEvaluator
It also gives us a nice extension point for downstream projects. For example if a project like OpenJ9 wishes to re-implement monentEvaluator they have to manually whack in a tree evaluator table overwritten entry today which is quite convoluted:
https://github.com/eclipse/openj9/blob/d61567612953eafa234c124c3cc875f2b589f513/runtime/compiler/x/codegen/J9TreeEvaluator.cpp#L598-L599
The only downside I see is an increase in footprint. This I will measure once I prototype the change. If the footprint increase is small, does anyone oppose to this work being done? (edited)
👍
2

31 replies

---

leonardo2718 7 days ago
I'm in support 🙂

---

mstoodle 7 days ago
it would bake in an assumption that an opcode's evaluator cannot change except at build time. It's not like we exploit that flexibility now (other than the OpenJ9 "whacking" you described, which I agree is an undesirable way to accomplish it), but it is something that goes away with your proposed scheme

---

mstoodle 7 days ago
i'm struggling to think of a really good reason to keep that flexibility, though

---

mstoodle 7 days ago
it could also be recovered by making an opcode evaluator function virtual

---

mstoodle 7 days ago
but that carries its own complexities that may be undesireable

---

Filip Jeremic 7 days ago
an opcode’s evaluator cannot change except at build time.
How about:

TR::Register *OMR::X86::AMD64::TreeEvaluator::BNDCHKEvaluator(TR::Node *node, TR::CodeGenerator *cg)
   {
   if (comp->getOption(TR_OverrideEvaluatorAtRuntime))
      {
      return TR::Foo::someOtherFunction(node, cg);
      }
      else
      {
      return TR::TreeEvaluator::badILOpEvaluator(node, cg);
      }
   }

(edited)

---

mstoodle 7 days ago
i agree, it could be done, it just gets messier than changing an entry in the evaluator table

---

Filip Jeremic 7 days ago
I don’t think we have an example of that today. I can’t think of a reason why someone may want to do that either, but it is definitely something we’d lose.

---

Filip Jeremic 7 days ago
Does such a loss of being able to override evaluators at runtime outweigh the conceptual simplicity and consistency across platforms which this change will introduce?

---

mstoodle 7 days ago
it wouldn't be anything where we would need to change every evaluator, or we may as well just extend the project and handle it that way. If it's something that only changes a few opcodes, then that's a small impact whereever we do it. I'm not sure the design space where we would want that kind of flexibility is large enough to really care about.

---

mstoodle 7 days ago
I would want to hear @0xdaryl’s opinion on the subject, though
👍

---

mstoodle 7 days ago
Does such a loss ....
It would depend on what abilities we lose for that simplicity. As an outrageous example, it's a lot simpler to build a compiler that only has to deal with word sized integers, but that doesn't match the use cases we care about.

---

Filip Jeremic 7 days ago
FYI the trick OpenJ9 pulls to overwrite the tree evaluator table can still be done. The only thing that changes is that if a project defines an opcode there has to be a TR::TreeEvaluator::Evaluator function at compile time. This is the default value populated in the auto-generated tree evaluator table. The project can still at runtime overwrite the value in the table the same way OpenJ9 currently does. (edited)

---

mstoodle 7 days ago
oh, ok, so you're not going to eliminate the table itself...i guess that makes more sense, doesn't it 🙂
✅

---

mstoodle 7 days ago
well, i was already leaning towards agreeing with you

---

mstoodle 7 days ago
still interested to hear Daryl's opinion, though

---

Filip Jeremic 7 days ago
If we still think it’s a bad idea to override at runtime, we can make the table:
https://github.com/eclipse/omr/blob/1f46e75cf508f47d2d2d8ce48678336f6d845f7f/compiler/codegen/OMRCodeGenerator.hpp#L1988
into a static const
compiler/codegen/OMRCodeGenerator.hpp:1988
static TR_TreeEvaluatorFunctionPointer _nodeToInstrEvaluators[];
https://github.com/eclipse/omr|eclipse/omreclipse/omr | Added by GitHub

---

mstoodle 7 days ago
yup

---

dsouzai 7 days ago
I think it's a bad idea to have the flexibility to change the evaluator at runtime. The flexibility would likely allow for a dev to use it for some "clever" (likely one-off) purpose which would either make the code much more confusing, or introduce subtle bugs. If, should this proposal go through, attempting to change the evaluator at runtime is a lot more complicated and messy, then that should be seen as a good indicator that it's best to extend the project to allow for the override to be done cleanly.

---

mstoodle 7 days ago
agree that if we can't think of any valid reasons to allow it, then starting position should be it can't change

---

mstoodle 7 days ago
i was trying to imagine something like a debug case where you could decide to wrap every evaluator call in traceOnEntry and traceOnExit output without having to weave it all into the source code, but that feels pretty contrived

---

mstoodle 7 days ago
-Xjit:traceTheCrapOutOfCodegenEvaluators

---

dsouzai 7 days ago
I think that should be doable with a debug table, something like DebugEvaluator which then calls Evaluator. Maybe i'm oversimplifying 😛

---

mstoodle 7 days ago
i say that mostly for humour content, but also because it could spark other ideas

---

Filip Jeremic 7 days ago
Or just override cg->evaluate() to print it before we dispatch evaluation to the evaluator via the tree evaluator table.
i.e. put that hypothetical debug code before an after this call:
https://github.com/eclipse/omr/blob/1f46e75cf508f47d2d2d8ce48678336f6d845f7f/compiler/codegen/NodeEvaluation.cpp#L181 (edited)
compiler/codegen/NodeEvaluation.cpp:181
reg = _nodeToInstrEvaluators[opcode](node, self());
https://github.com/eclipse/omr|eclipse/omreclipse/omr | Added by GitHub
👍

---

mstoodle 7 days ago
I did say I couldn't think of a good example...
😆

---

Jan 7 days ago
Maybe I'm missing something, but this way we'd also loose an (easy) way for using the same evaluator for multiple opcodes (something I do quite often in RISC-V codegen).
Sure, one can always do something like:

laddEvaluator(...) {
  return iaddEvaluator(...);
}  

---

Filip Jeremic 7 days ago
That’s right. It would have to be done this way.
Also sent to the channel

---

Filip Jeremic 7 days ago
Thinking about this some more I think there is a way to do it as a NOP change with some complexity with C macros. As an example for the TR::monent case where a downstream project wants to override the evaluator, rather than defining it in OMR as:
https://github.com/eclipse/omr/blob/1f46e75cf508f47d2d2d8ce48678336f6d845f7f/compiler/x/amd64/codegen/OMRTreeEvaluatorTable.hpp#L555
We would have to do, for each opcode:

#ifndef _monentEvaluator
#define _monentEvaluator TR::TreeEvaluator::badILOpEvaluator
#endif

Because downstream projects generate their tables by defining their own underscore defines, and they include the OMR ones at the end:
https://github.com/eclipse/openj9/blob/d61567612953eafa234c124c3cc875f2b589f513/runtime/compiler/x/amd64/codegen/TreeEvaluatorTable.hpp#L420
We could just do a #define _monentEvaluator TR::Whatever::foo in the downstream project and OMR will not override the value.
This is a more complex conceptual model, but it is free at runtime. Not sure which people prefer. Thoughts? (edited)

---

Filip Jeremic 7 days ago
This problem persists across both the Simplifier and VP tables by the way. To a lesser extent since those are cross platform.
Sounds to me like a topic to discuss on the Architecture call. @0xdaryl shall we pencil me in? (edited)

---

0xdaryl 6 days ago
The original design of the TreeEvaluatorTable in the original J9 compiler had const entries in the table. In order to re-use this table across different projects the const designation was removed and the table was patched once at compile-time by each project. That was the simplest and cheapest way of achieving that goal, not necessarily the best design. Like others, I can’t think of a good use case for needing to patch the evaluator table dynamically and I wouldn’t want thinking of those unlikely possibilities to detract from a nice clean design. If such a feature were required in the future by some project, the Node evaluate() function could be modified to select from a different table and in fact the evaluate() function itself could be specialized by a project so that this functionality doesn’t impact all other projects.
I like the uniformity across codegens that Filip proposes with the consistent naming of static functions. I am also in favour of using tooling to compose data structures across projects using a table generation approach if it provides the simplicity we need. There are data structures (like the evaluator tables, simplifier tables, VP, Options, instructions, etc.) that can be composed at build time using some form of meta data. Nazim has proven this can work by implementing an Options refactoring using this approach. One of the drawbacks of this are tool chain dependencies on something like Python, but requiring Python in your build pipeline isn’t such an unreasonable ask these days in my opinion. That’s why the Options code never landed because of downstream dependencies, but I think it is something to revisit as they are likely resolved on all platforms now.
I will add this to the agenda for discussion next week.

[fjeremic]

I've gone ahead and made the changes as proposed on the OMR Architecture meeting in #5720 and I also have a corresponding change in OpenJ9 at eclipse-openj9/openj9#11465 just to show how things look. A few notes to make which I'll summarize here:

• The change was prototyped only for AMD64 as an example.
• The changes are an absolute extreme of what will be done because there will never be more "stub" functions introduced. In fact a lot of the current functions which provide the "meat" of the evaluators can be inlined in to the generated *::TreeEvaluator::<opcode>Evaluator stubs generated. This will greatly reduce the number of functions we have.
• I measured the total footprint increase using an OpenJ9 build of the changes and came up with the following stats:

fjeremic@Filips-MacBook-Pro temp % find . -name libj9jit29.so -exec ls -al {} \;
-rwxr-xr-x@ 1 fjeremic  wheel  13247096 11 Dec 12:43 ./before/jre/lib/amd64/compressedrefs/libj9jit29.so
-rwxr-xr-x@ 1 fjeremic  wheel  13242992 11 Dec 12:42 ./before/jre/lib/amd64/default/libj9jit29.so
-rwxr-xr-x@ 1 fjeremic  wheel  13352376 11 Dec 16:01 ./after/jre/lib/amd64/compressedrefs/libj9jit29.so
-rwxr-xr-x@ 1 fjeremic  wheel  13344168 11 Dec 16:00 ./after/jre/lib/amd64/default/libj9jit29.so

fjeremic@Filips-MacBook-Pro temp % objdump -d ./after/jre/lib/amd64/compressedrefs/libj9jit29.so > asm.out
000000000083aac0 _ZN3OMR3X865AMD6413TreeEvaluator16BadILOpEvaluatorEPN2TR4NodeEPNS3_13CodeGeneratorE:
  83aac0: e9 4b 69 bd ff                jmp     -4363957 <_ZN3OMR13TreeEvaluator16badILOpEvaluatorEPN2TR4NodeEPNS1_13CodeGeneratorE>
  83aac5: 90                            nop
  83aac6: 66 2e 0f 1f 84 00 00 00 00 00 nopw    %cs:(%rax,%rax)

• The change will increase footprint of the library by 0.7% which is IMO miniscule. I confirmed this by using objdump as well to showcase what one of these stubs generates in terms of number of bytes.
• There is no need for a TreeEvaluatorTable.hpp in OpenJ9 anymore since everything is auto-generated transparently because the tree evaluator names are standardized based off the opcodes. Neat!

I'm looking for a quorum of whether we should forge ahead with such a change. Again although the change looks large, it is very mechanical (I'll post the script used to generate it all) and it is an extreme. As we merge this we can continue inlining evaluators in their right places and getting rid of the stub functions. The consistency will however be present across all backends.

Please let me know if you have any concerns/comments/suggestions.

[mstoodle]

Thanks for your efforts to prototype the proposed direction, @fjeremic. Would you be able to quickly gauge the impact to the size of the JitBuilder library with this change?

[fjeremic]

Thanks for your efforts to prototype the proposed direction, @fjeremic. Would you be able to quickly gauge the impact to the size of the JitBuilder library with this change?

Here is the before and after on macOS:

Before:
-rw-r--r--   1 fjeremic  staff  31494168 30 Apr 13:25 libjitbuilder.a

After:
-rw-r--r--   1 fjeremic  staff  31723632 30 Apr 13:01 libjitbuilder.a

This is a 0.7% increase. The same as with OpenJ9 JIT library unsurprisingly. I used the same commit as in #5720 to build and get the size if you want to reproduce.

[mstoodle]

Sorry, missed this comment. The size looks pretty large: is that a debug build measurement?

[fjeremic]

Sorry, missed this comment. The size looks pretty large: is that a debug build measurement?

No, it is the same way we build on our farm, i.e. cmake -C ../cmake/caches/Travis.cmake ..

Edit:

Just tried it again and had the same outcome. Here is the exact set of commands on macOS:

git clone --depth 1 https://github.com/eclipse/omr.git
cd omr
mkdir build
cd build
cmake -C ../cmake/caches/Travis.cmake ..
make -j16
ls -alrt jitbuilder/
objdump --syms jitbuilder/libjitbuilder.a | wc -l

Here is the output of the last two commands:

fjeremic@Filips-MacBook-Pro build % ls -alrt jitbuilder/
total 62216
-rw-r--r--   1 fjeremic  staff    476632 13 May 14:10 Makefile
-rw-r--r--   1 fjeremic  staff      1418 13 May 14:10 cmake_install.cmake
-rw-r--r--   1 fjeremic  staff       766 13 May 14:10 CTestTestfile.cmake
drwxr-xr-x   5 fjeremic  staff       160 13 May 14:10 CMakeFiles
drwxr-xr-x  35 fjeremic  staff      1120 13 May 14:10 ..
-rw-r--r--   1 fjeremic  staff  31365152 13 May 14:13 libjitbuilder.a
drwxr-xr-x   8 fjeremic  staff       256 13 May 14:13 .
drwxr-xr-x  54 fjeremic  staff      1728 13 May 14:13 release
fjeremic@Filips-MacBook-Pro build % objdump --syms jitbuilder/libjitbuilder.a | wc -l
   90900

[mstoodle]

Ok, you're right, I'm sorry. I misread the digits. Thanks for following up on that Filip. I can live with 0.7% increase.

[fjeremic]

The work is now fully finished in #5720 and eclipse-openj9/openj9#11465.