This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Fold A|B | (A^B) --> A|B
ClosedPublic

Authored by MehrHeidar on Nov 14 2021, 1:05 PM.

Details

Summary

This patch adds the following fold opportunity:
A|B | (A^B) --> A|B

that is reported here : https://bugs.llvm.org/show_bug.cgi?id=52479

https://alive2.llvm.org/ce/z/33-My-

Test cases with base results are added in D113860

Diff Detail

Event Timeline

MehrHeidar created this revision.Nov 14 2021, 1:05 PM
MehrHeidar requested review of this revision.Nov 14 2021, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2021, 1:05 PM
foad added a comment.Nov 15 2021, 12:29 AM

This needs some lit tests.

llvm/lib/Analysis/InstructionSimplify.cpp
2270

Just curious: don't we have matchers that will automatically match both the swapped and non-swapped forms of a commutative op?

Replace m_Xor with m_c_Xor

MehrHeidar added inline comments.Nov 15 2021, 5:34 AM
llvm/lib/Analysis/InstructionSimplify.cpp
2270

I think there are matchers which can match both swapped operands of a commutive operands, I assume the c letter in m_c_Xor and m_c_Or for instance is standing for commutive. However, here I needed to match the operands of outer or instruction. So, I needed the commuted version as well. I am not sure, if there is any other way of doing that, because in all other folding cases implemented InstSimplify, the commuted version is added.

Also, I have added the lit test cases here D113860, to pre-commit and rebase this patch so we can show the changes.

Thanks for your comment.

foad added inline comments.Nov 15 2021, 5:51 AM
llvm/lib/Analysis/InstructionSimplify.cpp
2270

Sorry I had not noticed that you were already using m_c_Or in the first version of the patch. I think that is fine, and there is probably no need to use m_c_Xor as well?

Rebasing after test cases are added

rampitec added inline comments.Nov 15 2021, 8:43 AM
llvm/lib/Analysis/InstructionSimplify.cpp
2270

There is no need to use commuted matcher for the first pattern, it will match anyway.

2277

Same here, you can use just m_Xor.

Revert m_c_Xor to m_Xor

MehrHeidar marked 3 inline comments as done.Nov 15 2021, 9:13 AM
MehrHeidar added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
2270

Thanks for your suggestion, right. I replaced it with m_Xor

MehrHeidar marked an inline comment as done.Nov 15 2021, 9:44 AM

Do you think the patch looks good now? @rampitec @foad
Thanks

rampitec added inline comments.Nov 15 2021, 10:20 AM
llvm/test/Transforms/InstSimplify/or.ll
456 ↗(On Diff #387291)

I think you need 2 more tests with commuted 'or' operands to cover 2 m_c_Or patterns.

Add two more test cases for commuted or

MehrHeidar marked an inline comment as done.Nov 15 2021, 11:47 AM
MehrHeidar added inline comments.
llvm/test/Transforms/InstSimplify/or.ll
456 ↗(On Diff #387291)

I added two more test cases as per your suggestion.
Thanks

MehrHeidar marked an inline comment as done.Nov 15 2021, 11:48 AM
rampitec accepted this revision.Nov 15 2021, 12:22 PM

LGTM. Do you need help to push this?

This revision is now accepted and ready to land.Nov 15 2021, 12:22 PM

LGTM. Do you need help to push this?

Yes, I don't have commit access yet. If it's possible, can you please commit it?

Mehrnoosh Heidarpour
email: mehrnoosh.heidarpour@huawei.com

This revision was automatically updated to reflect the committed changes.

@rampitec I think you committed this patch with your name and not mine!
I wanted you to only commit it on behalf of me. Right now, it's your patch in the repository!

@rampitec I think you committed this patch with your name and not mine!
I wanted you to only commit it on behalf of me. Right now, it's your patch in the repository!

That's what is there: (authored by MehrHeidar, committed by rampitec): https://reviews.llvm.org/rG193c40e9667ca2b173232b393fc72ea9e4944aa3

@rampitec I think you committed this patch with your name and not mine!
I wanted you to only commit it on behalf of me. Right now, it's your patch in the repository!

That's what is there: (authored by MehrHeidar, committed by rampitec): https://reviews.llvm.org/rG193c40e9667ca2b173232b393fc72ea9e4944aa3

True, that's what is there. It's under your name and not mine anymore!
Although, it was authored by me, I am only at the last line of commit message. You have not used my email address and used your own, this is your patch now.

@rampitec I think you committed this patch with your name and not mine!
I wanted you to only commit it on behalf of me. Right now, it's your patch in the repository!

That's what is there: (authored by MehrHeidar, committed by rampitec): https://reviews.llvm.org/rG193c40e9667ca2b173232b393fc72ea9e4944aa3

True, that's what is there. It's under your name and not mine anymore!
Although, it was authored by me, I am only at the last line of commit message. You have not used my email address and used your own, this is your patch now.

@rampitec I think you committed this patch with your name and not mine!
I wanted you to only commit it on behalf of me. Right now, it's your patch in the repository!

That's what is there: (authored by MehrHeidar, committed by rampitec): https://reviews.llvm.org/rG193c40e9667ca2b173232b393fc72ea9e4944aa3

True, that's what is there. It's under your name and not mine anymore!
Although, it was authored by me, I am only at the last line of commit message. You have not used my email address and used your own, this is your patch now.

I have to admit I have no idea how to push from my account and use a different name.

@rampitec That was not fair, but Sometimes it happens, I think you made a mistake.

@rampitec Can you revert it?

@rampitec Can you revert it?

Sure, if you so wish.

@rampitec Can you revert it?

Sure, if you so wish.

Yes please. Thanks, I can ask another one to commit, maybe they know the trick to use my email address.

rampitec reopened this revision.Nov 15 2021, 3:04 PM
This revision is now accepted and ready to land.Nov 15 2021, 3:04 PM

@rampitec, use git commit --author="Mehrnoosh Heidarpour <mehrnoosh.heidarpour@huawei.com>". If you're --amending you might also need --reset-author.

Although, it was authored by me, I am only at the last line of commit message. You have not used my email address and used your own, this is your patch now.

When LLVM was still in SVN it was impossible to commit under a different name, so it's actually quite common to have the actual author named only in the message.

@rampitec, use git commit --author="Mehrnoosh Heidarpour <mehrnoosh.heidarpour@huawei.com>". If you're --amending you might also need --reset-author.

Although, it was authored by me, I am only at the last line of commit message. You have not used my email address and used your own, this is your patch now.

When LLVM was still in SVN it was impossible to commit under a different name, so it's actually quite common to have the actual author named only in the message.

Thank you, I didn't know it will be reopened, I abandoned the other patch.

This revision was automatically updated to reflect the committed changes.

@rampitec, use git commit --author="Mehrnoosh Heidarpour <mehrnoosh.heidarpour@huawei.com>". If you're --amending you might also need --reset-author.

Thanks, good to know!