{"payload":{"feedbackUrl":"https://github.com/orgs/community/discussions/53140","repo":{"id":186445498,"defaultBranch":"master","name":"openj9-omr","ownerLogin":"jdmpapin","currentUserCanPush":false,"isFork":true,"isEmpty":false,"createdAt":"2019-05-13T15:19:24.000Z","ownerAvatar":"https://avatars.githubusercontent.com/u/22403820?v=4","public":true,"private":false,"isOrgOwned":false},"refInfo":{"name":"","listCacheKey":"v0:1705687167.0","currentOid":""},"activityList":{"items":[{"before":null,"after":"2c2cc01114db6a91990298f583417d7bb3eaca0a","ref":"refs/heads/bndchk-versioning-test","pushedAt":"2024-01-19T17:59:27.000Z","pushType":"branch_creation","commitsCount":0,"pusher":{"login":"jdmpapin","name":"Devin Papineau","path":"/jdmpapin","primaryAvatarUrl":"https://avatars.githubusercontent.com/u/22403820?s=80&v=4"},"commit":{"message":"Fix versioning test against final index value for BNDCHK\n\nPreviously this versioning test would do either a strict or non-strict\ncomparison depending on a few factors:\n- isLoopDrivingInductionVariable,\n- isIndexChildMultiplied, and\n- the opcode of _loopTestTree (without consulting reverseBranch).\n\nTo deal with all of this variation there were multiple ad hoc\nadjustments of +1 or -1 at different points in the determination of\nmaxValue (which is really supposed to be the last value).\n\nThis state of affairs led to bugs, notably when using a non-strict loop\ntest together with an index expression that contains a multiplication,\nbut also in other scenarios that I have not precisely characterized.\n\nNow this versioning test is always (ifiucmpge maxValue arrayLength) for\nincreasing index expressions and (ificmplt maxValue 0) for decreasing\nones, with no regard to isLoopDrivingInductionVariable, etc., and the\ncorresponding ad hoc adjustments are eliminated from maxValue. Rather\nthan always take the ceiling and try to compensate for non-strict loop\ntests after the fact, we now use the floor in the case of non-strict\nloop tests and (ceiling - 1) otherwise, as described in the comment in\nthe extremum versioning path in buildConditionalTree(). (Note though\nthat instead of subtracting 1 before multiplying by incrJNode, this\nchange subtracts additiveConstantInStore afterward, which has the same\neffect.)\n\nAdditionally, if the index expression was based on a derived IV and\ninvolved a substitution via isDependentOnInductionVariable(), then this\nversioning test would load the temporary that's defined within the loop\ninstead of the initial value of the IV. The temp might be uninitialized,\nresulting in a versioning test that can non-deterministically pass even\nif the bound check should fail. It's not much better either if the temp\nhappens to be initialized, since its value won't have come from the\ndefinition within the loop and is most likely entirely unrelated to the\ninitial value of the IV. This commit changes the versioning test to load\nthe derived IV instead of the temporary.\n\nFinally, it was also possible for a later versioning test to load an\nuninitialized temporary when trying to rule out overflow in a\nmultiplication that appears in the index expression. To prevent this,\nversioning is now disallowed if it would be necessary to substitute\nbeneath a multiplication.\n\nWith these changes the versioning test is still not precise, which I\nbelieve is due to a failure to account properly for different possible\norderings of operations in the loop and possibly also to account for the\ndifference between the initial value of a derived IV and the value of a\ntemp based on it in the first iteration (e.g. tmp=iv+1).\n\nHowever, it appears to be conservative. I have checked a large number of\ngenerated test cases with different combinations of features:\n- Do-while loops vs while loops (reverseBranch)\n- Strict vs. non-strict loop conditions\n- Increasing primary IV (with <, <= comparisons) vs. decreasing (>, >=)\n- Accessing every element vs. every other element\n- Getting every other element by stepping by 2 vs. multiplying by 2\n- Even vs. odd array length (when accessing every other element)\n- Accessing even vs. odd indices (when accessing every other element)\n- Accessing elements in increasing vs. decreasing order of index\n- Indexing based on primary vs. derived IV\n- Stepping the primary IV by -3, -1, +1, or +3 (if using a derived IV)\n- Adding an offset in the index expression vs. not adding one\n- Adding an offset before multiplying vs. afterward (if multiplying)\n- Using a temp (isDependentOnInductionVariable()) vs. no temp\n- Setting the temp to the IV or to the index (if using a temp)\n- Different initial values for the IV(s), as consistent with the above\n- Every usable permutation of the statements in the loop body\n\nIn every case I had a script determine the widest loop bound that avoids\nfailing a bound check. I ran each test case three times:\n- once with the precise bounds, which should not fail a bound check;\n- once with the initial value modified to cause a bound check to fail on\n  the first iteration; and\n- once with the loop bound modified to do an extra iteration at the end\n  and fail a bound check in the last iteration.\n\nWith this commit, all such test executions behaved as expected. In some\ncases the bound check was not versioned or execution went unnecessarily\nto the cold loop, but in those cases one of the following always held\nbefore this commit:\n- we already did the same thing, or\n- the test passed all bound checks when it should have failed one, or\n- there was a load of an uninitialized variable in the versioning test.\n\nMost loops in real programs compute the index based on the primary IV,\nwhich appears directly in the index expression, i.e. array[...i...]. For\nthese loops as they occur in the test cases described above, the only\neffect of this change is to correctly run bound checks when one should\nfail and to sometimes avoid going unnecessarily to the cold loop.","shortMessageHtmlLink":"Fix versioning test against final index value for BNDCHK"}},{"before":null,"after":"a336ce069801574654e2270e9470166316359fa0","ref":"refs/heads/ram-method-sequence","pushedAt":"2023-08-29T19:34:35.000Z","pushType":"branch_creation","commitsCount":0,"pusher":{"login":"jdmpapin","name":"Devin Papineau","path":"/jdmpapin","primaryAvatarUrl":"https://avatars.githubusercontent.com/u/22403820?s=80&v=4"},"commit":{"message":"Stop special-casing unevaluated aconst in Power and ARM linkage\n\nInstead, simply evaluate it like we do for other arguments.\n\nThese were the only two places where a TR_RamMethodSequence relocation\ncould be generated for a method other than the outermost method, which\nis not expected. All remaining uses of TR_RamMethodSequence are for\ncompiledMethodSymbol, which always represents the outermost method.","shortMessageHtmlLink":"Stop special-casing unevaluated aconst in Power and ARM linkage"}},{"before":"a336ce069801574654e2270e9470166316359fa0","after":null,"ref":"refs/heads/ram-method-sequence","pushedAt":"2023-08-29T19:34:16.000Z","pushType":"branch_deletion","commitsCount":0,"pusher":{"login":"jdmpapin","name":"Devin Papineau","path":"/jdmpapin","primaryAvatarUrl":"https://avatars.githubusercontent.com/u/22403820?s=80&v=4"}},{"before":null,"after":"a336ce069801574654e2270e9470166316359fa0","ref":"refs/heads/ram-method-sequence","pushedAt":"2023-08-29T19:29:40.000Z","pushType":"branch_creation","commitsCount":0,"pusher":{"login":"jdmpapin","name":"Devin Papineau","path":"/jdmpapin","primaryAvatarUrl":"https://avatars.githubusercontent.com/u/22403820?s=80&v=4"},"commit":{"message":"Stop special-casing unevaluated aconst in Power and ARM linkage\n\nInstead, simply evaluate it like we do for other arguments.\n\nThese were the only two places where a TR_RamMethodSequence relocation\ncould be generated for a method other than the outermost method, which\nis not expected. All remaining uses of TR_RamMethodSequence are for\ncompiledMethodSymbol, which always represents the outermost method.","shortMessageHtmlLink":"Stop special-casing unevaluated aconst in Power and ARM linkage"}}],"hasNextPage":false,"hasPreviousPage":false,"activityType":"all","actor":null,"timePeriod":"all","sort":"DESC","perPage":30,"cursor":"Y3Vyc29yOnYyOpK7MjAyNC0wMS0xOVQxNzo1OToyNy4wMDAwMDBazwAAAAPkO8_9","startCursor":"Y3Vyc29yOnYyOpK7MjAyNC0wMS0xOVQxNzo1OToyNy4wMDAwMDBazwAAAAPkO8_9","endCursor":"Y3Vyc29yOnYyOpK7MjAyMy0wOC0yOVQxOToyOTo0MC4wMDAwMDBazwAAAAN1LGFU"}},"title":"Activity · jdmpapin/openj9-omr"}