Page MenuHomePhabricator

[DAGCombiner] Peek through vector concats when trying to combine shuffles.
ClosedPublic

Authored by deadalnix on Sep 29 2019, 11:01 AM.

Details

Summary

This combine showed up as needed when exploring the regression when processing the DAG in topological order.

Diff Detail

Event Timeline

deadalnix created this revision.Sep 29 2019, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2019, 11:01 AM

Rebase and ping.

This is desperately missing some comments explaining what is going on.

deadalnix updated this revision to Diff 226018.Oct 22 2019, 4:03 AM

Add a few comments

xbolva00 added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16407

SDValue Op :

?

16409

O is similar to 0.

So ‘Op’?

lebedev.ri accepted this revision.Fri, Nov 22, 3:53 AM

Okay, this seems pretty unusual, but correct.
Would be good for @RKSimon to take a look, but LGTM.

I don't have comments as to the algorithm, but i find
the comments/variable names to be somewhat confusing.
Some 'recommendations':

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
16369–16370

... where shuffle operands may be CONCAT_VECTORS.

16375

I forget, by now Mask.size() == Vec.getValueType().getVectorNumElements()?

16384–16385

This is no longer X offset, this is what was most confusing to me.
I think this needs to be renamed to something else.

16410–16411

I think this needs a defensive/explanatory assert that by the end
of the loop ArgOffset is back to the original value of ArgOffset.

This revision is now accepted and ready to land.Fri, Nov 22, 3:53 AM
RKSimon accepted this revision.Fri, Nov 22, 8:15 AM

LGTM - I was thinking that combineX86ShufflesRecursively could handle this but its probably better to get it dealt with before lowering happens.

deadalnix updated this revision to Diff 230736.Fri, Nov 22, 3:32 PM

Address some of the comments

Alright, this is the first time I have to commit something with the new github process, and I have to admit, I do not know how to do it - or if I can do it at all. Running git llvm push ask me for a username and password, and my old one do not work. What's the next step for me here?

Alright, this is the first time I have to commit something with the new github process, and I have to admit, I do not know how to do it - or if I can do it at all. Running git llvm push ask me for a username and password, and my old one do not work. What's the next step for me here?

Did you ever pushed anything to github before, to any other repository?
Did you clone https://github.com/llvm/llvm-project/ via HTTPS or SSH? (should be former!)
It is likely asking for your github login and password.

You may *really* want to follow https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line to create credentials with which you can *only* commit, but do nothing else

Alright, this is the first time I have to commit something with the new github process, and I have to admit, I do not know how to do it - or if I can do it at all. Running git llvm push ask me for a username and password, and my old one do not work. What's the next step for me here?

Did you ever pushed anything to github before, to any other repository?
Did you clone https://github.com/llvm/llvm-project/ via HTTPS or SSH? (should be former!)
It is likely asking for your github login and password.

You may *really* want to follow https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line to create credentials with which you can *only* commit, but do nothing else

Hi,

Thanks for helping me out here. So I did commit on LLVM using the pre-github workflow (using git-svn), and did commit to github before in my own projects. I did not yet use the intersection of the two.

Here is what I'm getting when I'm trying to push that patch:

$ git llvm push
`git fetch https://github.com/llvm/llvm-project.git master` printed to stderr:
From https://github.com/llvm/llvm-project
 * branch                    master     -> FETCH_HEAD
Pushing 1 commit:
  0252f080c7c [DAGCombiner] Peek through vector concats when trying to combine shuffles.
Username for 'https://github.com': deadalnix
Password for 'https://deadalnix@github.com':
`git push https://github.com/llvm/llvm-project.git 0252f080c7c01e7493c7d6c9bb9ed8701469c4a8:master` returned 128
remote: Permission to llvm/llvm-project.git denied to deadalnix.
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': The requested URL returned error: 403

I tried to generate a token as suggested, and tried different permission settings for the token, including granting all permission to make sure there wasn't something I was missing, and I always get the same result.

I cant find your nick here: https://llvm.org/viewvc/llvm-project/meta/trunk/github-usernames.txt?view=markup .

"If you had commit access to SVN and would like to request commit access to GitHub, please email llvm-admin with your SVN username and GitHub username."

llvm-admin@lists.llvm.org

I cant find your nick here: https://llvm.org/viewvc/llvm-project/meta/trunk/github-usernames.txt?view=markup .

"If you had commit access to SVN and would like to request commit access to GitHub, please email llvm-admin with your SVN username and GitHub username."

llvm-admin@lists.llvm.org

That indeed would be my next guess

That is indeed the step I was missing. Thanks everybody for getting me up to speed.

This revision was automatically updated to reflect the committed changes.