Add array constant folding #725

rodrigovalle · 2021-10-07T00:19:26Z

Fixes #681

I created a new ConstFoldArrayElems folder to visit ArrayElemsCopied and ArrayElemsValues while holding onto the original ArrayExpr object that we recurse into.

philberty · 2021-10-07T13:03:32Z

Hi thanks so much for this bug fix! You should be able to find a clang-format file here: https://github.com/Rust-GCC/gccrs/blob/master/contrib/clang-format

You should be able to copy this to the root directory and name it .clang-format and it can be picked up if your using language server modes like lsp-mode in emacs for example.

philberty

This looks good so far but we need to add a test case to the testsuite for this before we can merge it. I would suggest adding this:

const TEST: [i32; 16] = [2, 6, 3, 10, 7, 0, 4, 13, 1, 11, 12, 5, 9, 14, 15, 8];

to gcc/testsuite/rust/compile/torture/array_const_fold_1.rs you may need to add a dg_warning annotation for unused but you can find examples for that in the testsuite.

This PR serves as the first basis for folding arrays and array index expression which is really cool. Did you have any issues with make-check-rust?

Thanks

edit: i just read your message below the template, just wish to say thanks and great work. When we merge this you will need to update your PR comment to remove the PR template stuff otherwise bors will add it to the merge commit message.

gcc/rust/typecheck/rust-hir-const-fold.cc

philberty · 2021-10-07T13:07:32Z

gcc/rust/typecheck/rust-hir-const-fold.cc

+  elems.iterate([&](HIR::Expr* inner) {
+    indices.push_back(index++);
+    Bexpression* bexpr = ConstFoldExpr::fold(inner);
+    if (bexpr) {


There is also an open issue about mixing error_mark_node and nullptr here: #692

We should really not have to do a nullptr check here if we are correctly using error_mark_node but this could be fine for now in this PR.

gcc/rust/typecheck/rust-hir-const-fold.h

philberty · 2021-10-07T13:13:15Z

@CohenArthur or @dkm how do you invoke clang-format?

dkm · 2021-10-07T18:07:32Z

@CohenArthur or @dkm how do you invoke clang-format?

I usually do this:

$ ln -s contrib/clang-format .clang-format
...
$ git add some_files
$ git clang-format
<this adds style changes needed for staged changes as unstaged changes >
$ git add some_files
$ git commit .....

With debian, the clang-format packages come with the git command installed correctly. You can also install several versions of clang-format, each package installs the corresponding git command: git clang-format-11 ... so you can easily deal with different versions :)

HTH

CohenArthur

Super cool work :) Looking forward to the tests. Regarding clang-format, I downloaded a clang-format-10 binary from the CI's clang-format action which keeps a list of them. We have been having issues with clang-format versions, and I believe the repo is still using clang-format-10. I think @dkm's solution is really good, and especially if your git package comes with different binaries like his. This is what I would recommend.

gcc/rust/typecheck/rust-hir-const-fold.h

rodrigovalle · 2021-10-11T08:57:17Z

Hi! Thanks for all of the feedback and help with this PR, I added the test and managed to get clang format working the way dkm described. I think the remaining clang format failures don't seem related to the lines of code that I changed?

I addressed the following in the new changes:

added torture/array_const_fold.rs test case
added another commit which removes ArrayElemsValues::iterate and replaces it with the suggested get_values() method
replace nullptr with error_expression()
remove undescores from member variable names

CohenArthur · 2021-10-11T09:02:54Z

@rodrigovalle Regarding the clang-format failures, I had that happen as well. As you point out, they are not part of your changes, but you still clang-formatted the rest of the code. I think there are some weird edge cases with our .clang-format file and the versions we use, between clang-format11 and 10, which might also happen or not happen because of our different distros, etc. I know I've had that issue in the past and now only run clang-format on my changes.

You could try running git clang-format between your commit and the one on master? As in run git clang-format 99c2830 0471803

CohenArthur · 2021-10-11T09:07:48Z

gcc/rust/typecheck/rust-hir-const-fold.cc

+void
+ConstFoldArrayElems::visit (HIR::ArrayElemsCopied &elems)
+{
+  // TODO


I think I missed this in my first review. Is this something you plan on doing in this PR or no?

Oh I forgot to mention this, yes I have an implementation for this on a different branch locally. It doesn't work yet, I got stuck because after calling ConstFoldExpr::fold() I have a Bexpression* that I need to convert into a size_t so that I can populate the indices vector to pass into array_constructor_expression.

I was thinking it might make sense to do add that later in a different PR, it seems like it might need a new type of array_constructor_expression method. I was looking at VEC_DUPLICATE_EXPR in the GCC GENERIC docs but my knowledge there is limited.

Happy to be wrong about this though, maybe there is a way to directly pull a size_t out :)

Yeah this is a perfect thing to do in a separate PR. I prefer lots of smaller PR's so long as we don't regress the testsuite.

rodrigovalle · 2021-10-11T09:16:53Z

I will experiment some more with clang-format tomorrow, I just ran this from HEAD = 0471803

$ git clang-format --diff 99c2830 0471803

And I had no output, status code 0.

dafaust

LGTM, thanks!

And fwiw, I have also been running into the same clang-format issues in #726

dkm · 2021-10-12T16:59:29Z

Also, there's still the copyright assignment / DCO to be checked. What are you planning on using @rodrigovalle ?

rodrigovalle · 2021-10-13T02:57:20Z

I opted for DCO by adding the Signed-off-by: tag to my commit message. Let me know if there's anything else I should clean up. I noticed that the first commit here (1fe5af2) duplicates some of the work in 727; I don't mind waiting for that one to land first and then rebasing the second patch on top of master if that works out better.

I'll keep looking at folding ArrayElemsCopied in the meantime.

philberty · 2021-10-13T09:17:53Z

gcc/rust/typecheck/rust-hir-const-fold.cc

+    {
+      indices.push_back (index++);
+      Bexpression *bexpr = ConstFoldExpr::fold (value.get ());
+      if (bexpr)


we have an open issue here about this ideally we should not be checking for nullptr and instead checking for error_mark_node.

The idea is that the constant folding can continue on with errors.

I think you should remove the gcc_unreachable() branch and check. The #692 should be the issue to fix this.

philberty

I think to merge this you just need to remove that gcc_unreachable branch otherwise this LGTM

philberty · 2021-10-15T14:50:33Z

bors try

bors · 2021-10-15T14:50:36Z

try

Merge conflict.

philberty · 2021-10-15T15:05:44Z

@rodrigovalle I think you likely just need to drop the first commit from your PR since this ended up getting merged in #727. I can fix up this for you if you like so we can include this contribution on the Monday status report.

rodrigovalle · 2021-10-16T02:46:14Z

Ok thanks! I've removed the nullptr check and the first commit so it should merge nicely onto master. I also have a patch to fold ArrayElemsCopied but I will PR that separately.

Create a new ConstFoldArrayElems folder to visit ArrayElemsValues while holding onto the original ArrayExpr object that we recurse into. Fixes Rust-GCC#681 Signed-off-by: Rodrigo Valle <rdan.valle@gmail.com>

rodrigovalle · 2021-10-16T02:56:55Z

This second push was to remove the nullptr check after we call ConstFoldArrayElems::fold since we use error_expression as the default return value.

philberty

Awesome work

philberty · 2021-10-16T19:28:29Z

bors r+

philberty · 2021-10-16T19:29:03Z

@rodrigovalle this is fantastic thanks for rebasing this :D

bors · 2021-10-16T19:57:02Z

Build succeeded:

rodrigovalle closed this Oct 7, 2021

rodrigovalle reopened this Oct 7, 2021

rodrigovalle marked this pull request as draft October 7, 2021 01:08

philberty added the bug label Oct 7, 2021

philberty added this to the Control Flow 2 - Pattern Matching milestone Oct 7, 2021

philberty requested changes Oct 7, 2021

View reviewed changes

philberty requested review from CohenArthur, dafaust and thomasyonug October 7, 2021 13:14

CohenArthur approved these changes Oct 8, 2021

View reviewed changes

gcc/rust/typecheck/rust-hir-const-fold.h Outdated Show resolved Hide resolved

rodrigovalle force-pushed the master branch 2 times, most recently from 4979a7d to 0471803 Compare October 11, 2021 08:44

rodrigovalle marked this pull request as ready for review October 11, 2021 08:59

CohenArthur approved these changes Oct 11, 2021

View reviewed changes

rodrigovalle changed the title ~~[WIP] Add array constant folding~~ Add array constant folding Oct 11, 2021

dafaust approved these changes Oct 11, 2021

View reviewed changes

rodrigovalle force-pushed the master branch from 0471803 to 1c13081 Compare October 13, 2021 02:53

philberty mentioned this pull request Oct 13, 2021

Remove AST::BlockExpr lambda and add get_statements #728

Merged

philberty reviewed Oct 13, 2021

View reviewed changes

philberty requested changes Oct 13, 2021

View reviewed changes

philberty assigned rodrigovalle Oct 15, 2021

rodrigovalle force-pushed the master branch from 1c13081 to a3acaf1 Compare October 16, 2021 02:43

Add array constant folding

14a0f2c

Create a new ConstFoldArrayElems folder to visit ArrayElemsValues while holding onto the original ArrayExpr object that we recurse into. Fixes Rust-GCC#681 Signed-off-by: Rodrigo Valle <rdan.valle@gmail.com>

rodrigovalle force-pushed the master branch from a3acaf1 to 14a0f2c Compare October 16, 2021 02:55

philberty approved these changes Oct 16, 2021

View reviewed changes

bors bot merged commit ec1e7c2 into Rust-GCC:master Oct 16, 2021

tschwinge mentioned this pull request Nov 5, 2021

Running clang-format locally [#242] #795

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add array constant folding #725

Add array constant folding #725

rodrigovalle commented Oct 7, 2021 •

edited

Loading

philberty commented Oct 7, 2021

philberty left a comment •

edited

Loading

philberty Oct 7, 2021

philberty commented Oct 7, 2021

dkm commented Oct 7, 2021

CohenArthur left a comment

rodrigovalle commented Oct 11, 2021

CohenArthur commented Oct 11, 2021

CohenArthur Oct 11, 2021

rodrigovalle Oct 11, 2021

rodrigovalle Oct 11, 2021

philberty Oct 13, 2021

rodrigovalle commented Oct 11, 2021

dafaust left a comment

dkm commented Oct 12, 2021

rodrigovalle commented Oct 13, 2021

philberty Oct 13, 2021

philberty left a comment

philberty commented Oct 15, 2021

bors bot commented Oct 15, 2021

philberty commented Oct 15, 2021

rodrigovalle commented Oct 16, 2021

rodrigovalle commented Oct 16, 2021

philberty left a comment

philberty commented Oct 16, 2021

philberty commented Oct 16, 2021

bors bot commented Oct 16, 2021

Add array constant folding #725

Add array constant folding #725

Conversation

rodrigovalle commented Oct 7, 2021 • edited Loading

philberty commented Oct 7, 2021

philberty left a comment • edited Loading

Choose a reason for hiding this comment

philberty Oct 7, 2021

Choose a reason for hiding this comment

philberty commented Oct 7, 2021

dkm commented Oct 7, 2021

CohenArthur left a comment

Choose a reason for hiding this comment

rodrigovalle commented Oct 11, 2021

CohenArthur commented Oct 11, 2021

CohenArthur Oct 11, 2021

Choose a reason for hiding this comment

rodrigovalle Oct 11, 2021

Choose a reason for hiding this comment

rodrigovalle Oct 11, 2021

Choose a reason for hiding this comment

philberty Oct 13, 2021

Choose a reason for hiding this comment

rodrigovalle commented Oct 11, 2021

dafaust left a comment

Choose a reason for hiding this comment

dkm commented Oct 12, 2021

rodrigovalle commented Oct 13, 2021

philberty Oct 13, 2021

Choose a reason for hiding this comment

philberty left a comment

Choose a reason for hiding this comment

philberty commented Oct 15, 2021

bors bot commented Oct 15, 2021

try

philberty commented Oct 15, 2021

rodrigovalle commented Oct 16, 2021

rodrigovalle commented Oct 16, 2021

philberty left a comment

Choose a reason for hiding this comment

philberty commented Oct 16, 2021

philberty commented Oct 16, 2021

bors bot commented Oct 16, 2021

rodrigovalle commented Oct 7, 2021 •

edited

Loading

philberty left a comment •

edited

Loading