jj10306 added a comment. In D105741#2920588 <https://reviews.llvm.org/D105741#2920588>, @vsk wrote:
> Hey @jj10306, thanks for working on this. > > @wallace @clayborg -- It seems like there are a ton of logic changes > introduced in this patch that are tested at too coarse a level. I'm not > confident in the changes being made here. For example, there's a bunch of > subtle work being done in `BasicSuperBlockMerge`, but only ~one opaque > high-level check that attempts to validate any of the logic: > `self.assertTrue(num_units_by_layer[1] == 383)`. Where does 383 come from? > How do we know that that's the right result? If there's a bug in > `BasicSuperBlockMerge`, would the regression test just look like changing the > magic 383 number? > > I'm not really comfortable with this being checked in, and would suggest a > revert until some more granular unit tests can be added to the patch. (Also > paging @JDevlieghere in case he has thoughts on the testing.) Apologies for this - will submit a patch with more comprehensive tests and other minor fixes this week! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105741/new/ https://reviews.llvm.org/D105741 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
