Page MenuHomePhabricator

[IRSim] Removing check that caused early matching of commutative instructions
ClosedPublic

Authored by AndrewLitteken on May 2 2022, 8:56 AM.

Details

Summary

When the first commutative instruction in a region using the same value in both positions was compared to a corresponding instruction with two different values, there was an early check that determined that since the values were new, it was true that these values acted in the same way structurally. If this was not contradicted later in the program, the regions were marked as similar. This removes that check, so that it is clear that the same value cannot be mapped to two different values.

Diff Detail

Event Timeline

AndrewLitteken created this revision.May 2 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 8:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
AndrewLitteken requested review of this revision.May 2 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 8:56 AM
paquette accepted this revision.May 2 2022, 10:41 AM

I think this is fine. I have a couple nits on the comments, but I won't hold up review over them.

llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp
2622

I think this comment would be clearer if you said "ensure that <instruction in test> is not mapped the same as <other instruction in test>"; it's not clear to me what's going on here right now.

2629

This is minor, but it would be really useful to have names for the variables here which describe what they're doing if possible.

E.g.

%commutative_inst = mul i64 undef, undef
...
This revision is now accepted and ready to land.May 2 2022, 10:41 AM
This revision was landed with ongoing or failed builds.May 9 2022, 8:59 PM
This revision was automatically updated to reflect the committed changes.

Hi,

This seems to solve the crash I reported in
https://github.com/llvm/llvm-project/issues/55131

Do you think it actually fixed the problem so that issue should be closed or is it just hidden?

Yes, this was written to fix that crash. Should have noted it in the commit message.