Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ target fails Performance/DropLoopEntryBranchInLRRule_4.txt #3959

Closed
parrt opened this issue Nov 14, 2022 · 5 comments
Closed

C++ target fails Performance/DropLoopEntryBranchInLRRule_4.txt #3959

parrt opened this issue Nov 14, 2022 · 5 comments

Comments

@parrt
Copy link
Member

parrt commented Nov 14, 2022

It hits nonlinearity OR it is merging incorrectly. I suspect the latter.

Run

$ cd ~/antlr/code/antlr4/runtime/Cpp
$ cmake . -DTRACE_ATN=ON -DCMAKE_BUILD_TYPE=Debug
$ make  # which makes dist/libantlr4-runtime.dylib

Unzip the file test.zip into a test dir to get test. Then from test dir:

$ ln -s ~/antlr/code/antlr4/runtime/Cpp/dist/libantlr4-runtime.dylib 
$ clang++ -g -std=c++17 -I /Users/parrt/antlr/code/antlr4/runtime/Cpp/runtime/src -L. -lantlr4-runtime *.cpp
$ ./a.out input

Comparing Java / C++ trace, we see Java reuses a context but C++ doesn't:

Screen Shot 2022-11-13 at 5 50 43 PM

If we have merged #3817 then you can get with output this assuming ~/tmp/bug/input is your test dir:

$ ~/antlr/code/antlr4/runtime-testsuite
$ bash ~/antlr/code/antlr4/scripts/traceatn.sh ~/tmp/bug/T.g4 stat -target Java ~/tmp/bug/input > /tmp/java
$ bash ~/antlr/code/antlr4/scripts/traceatn.sh ~/tmp/bug/T.g4 stat -target Cpp ~/tmp/bug/input > /tmp/cpp
$ diff /tmp/java /tmp/cpp
@parrt
Copy link
Member Author

parrt commented Nov 14, 2022

@kaby76 more fun. Note the diff above. :) I think that might expose the lack of reuse being the cause of speed issue.

@parrt
Copy link
Member Author

parrt commented Nov 14, 2022

The fix for #3845 exposed this issue.

@parrt
Copy link
Member Author

parrt commented Nov 14, 2022

After fixing my fix for #3845, we get this diff in the trace where C++ segv's instead of running forever (after the previous C++ nullptr fix).

Screen Shot 2022-11-14 at 5 48 34 PM

parrt added a commit that referenced this issue Nov 15, 2022
@parrt
Copy link
Member Author

parrt commented Nov 15, 2022

It dies here at line 170 in PredictionContext.h:

    PredictionContextType getContextType() const { return _contextType; }

caller is line 79 in ArrayPredictionContext.cpp

bool ArrayPredictionContext::equals(const PredictionContext &other) const {
  if (this == std::addressof(other)) {
    return true;
  }
  if (getContextType() != other.getContextType()) {   <==============

Ultimately this pops back to line 30 of ArrayPredictionContext.cpp where rhs.__ptr_ = 0x08, an invalid pointer:

  bool predictionContextEqual(const Ref<const PredictionContext> &lhs, const Ref<const PredictionContext> &rhs) {
    // parent PredictionContext pointers can be null during full context mode and
    // the ctxs are in an ArrayPredictionContext.  If both are null, return true
    // if just one is null, return false. If both are non-null, do comparison.
    if ( lhs == nullptr ) return rhs == nullptr;
    if ( rhs == nullptr ) return false; // lhs!=null and rhs==null
    return *lhs == *rhs;                // both nonnull         <=====================
  }

Coincidence that returnStates is a vector with one value in it: 8? Caller is line 92 of ArrayPredictionContext.cpp

          std::equal(parents.begin(), parents.end(), array.parents.begin(), predictionContextEqual);

almost as if we are passing in the return state as a pointer.

@parrt parrt added this to the 4.11.2 milestone Nov 22, 2022
parrt added a commit to parrt/antlr4 that referenced this issue Nov 22, 2022
parrt added a commit that referenced this issue Nov 22, 2022
@parrt
Copy link
Member Author

parrt commented Nov 22, 2022

Fixed by #3978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant