ReneEnjilian opened a new pull request, #2109: URL: https://github.com/apache/systemds/pull/2109
This pull request aims to improve test coverage of the classes RewriteAlgebraicSimplificationStatic/Dynamic, RewriteMatrixMultChainOptimizationSparse/Transpose and to identify potential issues there. ### 1. RewriteAlgebraicSimplificationStatic - **Covered rewrite rules:** simplifyCTableWithConstMatrixInputs, simplifyDistributiveBinaryOperation, simplifyBushyBinaryOperation, fuseBinarySubDAGToUnaryOperation, simplifyTraceMatrixMult, simplifyConstantSort, simplifyOrderedSort, simplifyTransposeAggBinaryChains, simplifyReplaceZeroOperation, fuseMinusNzBinaryOperation, fuseLogNzBinaryOperation, fuseLogNzUnaryOperation, simplifyCumsumReverse - **Identified Issues:** The rewrite `simplifyBushyBinaryOperation` is not entered by the corresponding statements ( (X*(Y*(Z%*%v))) or (X+(Y+(Z%*%v))) ). The conditional statements at the beginning of the rewrite seem not to match the input statements. Test methods that do not implement the rewrite are evaluated to _failed_. ### 2. RewriteAlgebraicSimplificationDynamic - **Covered rewrite rules:** fuseLeftIndexingChainToAppend, simplifyEmptyColMeans, simplifySumDiagToTrace, simplifyLowerTriExtraction, simplifyNnzComputation, pushDownBinaryOperationOnDiag, simplifyWeightedUnaryMM, simplifyWeightedSquaredLoss, simpliyWeightedSigmoidMMChains - **Identified Issues:** The rewrite `simplifyWeightedUnaryMM` is not executed for all 3 patterns it covers. The reason for this is that the if conditions seem not to match the actual statement patterns. I could correct some of them and then the rewrite was successfully executed. However, in this patch, I did not change the actual rewrite itself because this might result in unintentional consequences for other rewrites. The test methods not implementing the rewrite fail accordingly. Another rewrite that causes problems is `simplifyWeightedSigmoidMMChains`. Here, the missing transpose is not caught by the rewrite although it implements such logic. The reason for this bug is that the code misses an additional line like `tx = tx.getInput().get(0)`. This can be achieved by either adding this line or removing the `else`-clause that follows the transpose creation. I tried it and the missing transpose was caught and correctly replaced. Again, these changes are not part of this patch to avoid unintended consequences. The test methods not catching the missing transpose fail accordingly. ### 3. RewriteMatrixMultChainOptimizationSparse & RewriteMatrixMultChainOptimizationTranspose I also added testing to these classes and corrected existing test cases for _RewriteMatrixMultChainOptimizationTranspose_. These two classes had zero percent coverage because they were not linked to the `ProgramRewriter` class. Hence, they are never called. I added them both under `dynamicRewrites` below `RewriteMatrixMultChainOptimization` and `RewriteElementwiseMultChainOptimization`. Since `RewriteMatrixMultChainOptimizationSparse` overrides the standard `RewriteMatrixMultChainOptimization`, I wanted to be on the safe side and introduced in the `OptimizerUtils` a new flag called `ALLOW_ADVANCED_MMCHAIN_REWRITES`, which by default is set to false. I did this because I am not sure if it is actually intended to call RewriteMatrixMultChainOptimizationSparse/Transpose by default (e.g., maybe they contain bugs and are thus not part of the compilation). This flag is, in contrast to most other flags like `ALLOW_SUM_PRODUCT_REWRITES`, or `ALLOW_OPERATOR_FUSION`, set to false. I would be grateful for feedback on this issue. In subsequent PRs all the failing tests should be investigated and corrected (i.e., the corresponding rewrites). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@systemds.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
