BenefitInliner phase 3/3: Classes for doing nested knapsack algorithm and the inlining.

[xiacijie]

Issue: #5488

[mingweiarthurli]

Hi @jdmpapin, would you mind to review this pull request? Since this is the next phase of #5508 and there is no assignees now.

Please note this PR has been finished now. The title shows it is WIP because I'm not the original contributor and don't have the permission to change it.

Thank you in advance!😊

[karimhamdanali]

hey @jdmpapin , @vijaysun-omr I was wondering if you folks had a chance to review this PR? Unlike what the title suggests (which we don't have edit rights to change), it's no longer WIP and it is ready to be reviewed now that Phase 2 has been merged.

[jdmpapin]

I'll review soon

[karimhamdanali]

Thanks @jdmpapin!

[jdmpapin]

I started reviewing yesterday. Between other things, I hope to be done the review in the next few days

[mingweiarthurli]

I started reviewing yesterday. Between other things, I hope to be done the review in the next few days

Thank you very much!

[jdmpapin]

Sorry again, trying to get back to this soon

[mingweiarthurli]

Hi @jdmpapin, thank you for your review. I have finished resolving most review comments, except 4 comments still need some futher look since they may affect the core algorithm. I will finish those 4 comments ASAP and I will notify you once I finish. I also submmited finished part so you can start to review them if you want.

I submitted changes with another commit for now. I will squash those commits to 1 commit and force push to the repo after all changes are accepted.

[mingweiarthurli]

Hi @jdmpapin, I have made some new changes for the unsolved feedbacks. Could you please take a look? Thank you in advance.

Please ignore the macOS build check was failed since it failed due to a known issue of porttest.

[mingweiarthurli]

Hi @jdmpapin, I have made some changes to the unresolved and new comments. Please take a look.
I have also squashed all commits into one commit, so I think it is ready to be merged if all comments are solved and there isn't any new comment.

[mingweiarthurli]

Hi @jdmpapin, I fixed all new concerns. Please have a look. Thank you!

[jdmpapin]

Jenkins build all

[mingweiarthurli]

Hi @jdmpapin，I forgot to remove #includes <mutex> in the last commit when removing those std::call_once stuff. I removed it now, so the jenkins build checks should not fail anymore. Could you please run the jenkins again?

[jdmpapin]

Jenkins build all

[mingweiarthurli]

Hi @jdmpapin, seems almost all Jenkins build checks passed, excepts the macOS and linux riscv64.

For macOS, the console shows:

98% tests passed, 1 tests failed out of 49

Total Test time (real) = 238.41 sec

The following tests FAILED:
	  2 - TestJitBuilderAPIGenerator (Failed)

this is a known issue related to #7181.

For linux riscv64, the console shows:

...
15:05:15  29: Missing Implementation: Skipping test: ArraycmpLenNotEqualVariableLen/1219
15:05:15  29:     
15:05:15  29: Missing Implementation: Skipping test: ArraycmpLenNotEqualVariableLen/1220
15:05:15  29:     
15:05:15  29: Missing Implementation: Skipping test: ArraycmpLenNotEqualVariableLen/1221
15:05:15  29:     
15:05:15  29: Missing Implementation: Skipping test: ArraycmpLenNotEqualVariableLen/1222
15:05:15  29:     
15:05:15  29: Missing Implementation: Skipping test: ArraycmpLenNotEqualVariableLen/1223
15:05:15  29:     
15:05:15  29: �[0;32m[----------] �[m2448 tests from ArraycmplenTest/ArraycmplenNotEqualTest (3633 ms total)
15:05:15  29: 
15:05:17  29: �[0;32m[==========] �[m156911 tests from 173 test cases ran. (1476931 ms total)
15:05:17  29: �[0;32m[  PASSED  ] �[m156911 tests.
15:05:17  29: �[0;32m[  ALL TESTS PASSED  ] 
15:05:53  29: �[m
15:05:53  29/30 Test #29: comptest ..........................Child killed***Exception: 1563.04 sec
...
15:05:53  97% tests passed, 1 tests failed out of 30
15:05:53  
15:05:53  Total Test time (real) = 1971.26 sec
15:05:53  
15:05:53  The following tests FAILED:
15:05:53  	 29 - comptest (Child killed)
15:05:53  Errors while running CTest

looks like it is a silimar issue as mentioned at #5508 (comment), so I think this is also a known issue.

So I think this PR should be ready to be merged.

[mingweiarthurli]

Hi @jdmpapin, I think this PR should be ready to be merged. Would you mind to take a look for the final approval? Thank you!

[jdmpapin]

Jenkins build riscv

[keithc-ca]

I expect UMA builds need updating for this (e.g. omr/jitbuilder/build/files/common.mk should reference the new source files).

[mingweiarthurli]

Hi @keithc-ca, I will open another issue and PR later to add the new source files to this makefile. However, I'm not very sure what the "UMA" stands for. I saw some issues and comments mentioned it, but I cannot find any more info about it. Could you please tell me where I can find more info about the "UMA builds" so I can check if there is any other file also needs update.

[keithc-ca]

"UMA" is short for "Universal Makefile Adapter"; see https://github.com/eclipse-openj9/openj9/blob/master/sourcetools/com.ibm.uma/com/ibm/j9/uma/Main.java#L34.

In the context of OMR it involves using configure instead of cmake.

[mingweiarthurli]

Thnak you Keith! I will take a look for this problem.