pymlir icon indicating copy to clipboard operation
pymlir copied to clipboard

Simplify parsing ceildiv/floordiv/mod

Open shawwn opened this issue 3 years ago • 3 comments

I haven't been able to reproduce the problem described in these comments:

        # Pre-transform code to avoid parsing issues with ceildiv/floordiv/mod,
        # in which two symbols could be parsed as one legal symbol (due to
        # ignoring whitespace): "d0floordivs0"

This PR removes this transformation step.

I did run into a similar problem where arith.constant 0 : index was being parsed as arith.constant0. The root issue was in bare_id. I solved it by inlining the regexes, and introducing plain_id:

// Identifier syntax
//bare_id : (letter| underscore) (letter|digit|underscore|id_chars)*
plain_id : /[a-zA-Z_][a-zA-Z0-9_$]*/
bare_id : /[a-zA-Z_][a-zA-Z0-9_$.]*/
suffix_id : digits | bare_id

... and then replacing each instance of bare_id "." bare_id with plain_id "." plain_id. I can submit that as a PR if you'd like.

shawwn avatar May 15 '22 12:05 shawwn

Codecov Report

Merging #17 (32108aa) into master (88b81ab) will decrease coverage by 0.00%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage   94.44%   94.43%   -0.01%     
==========================================
  Files          15       15              
  Lines        1782     1779       -3     
==========================================
- Hits         1683     1680       -3     
  Misses         99       99              
Impacted Files Coverage Δ
mlir/parser.py 98.21% <ø> (-0.10%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 88b81ab...32108aa. Read the comment docs.

codecov-commenter avatar May 23 '22 12:05 codecov-commenter

Thanks for the PR! Interesting, I added this change specifically because things did not work. Could you please add a test that checks for it?

As for your other issue, I'm not sure if that is the best fix, but let's start another PR with a test and your fix and I'll try it out.

tbennun avatar May 23 '22 12:05 tbennun

@shawwn ping

tbennun avatar Jun 20 '22 12:06 tbennun