This is an archive of the discontinued LLVM Phabricator instance.

[llvm-diff] Implement diff of PHI nodes
ClosedPublic

Authored by void on Nov 18 2021, 5:03 PM.

Details

Diff Detail

Event Timeline

void requested review of this revision.Nov 18 2021, 5:03 PM
void created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 5:03 PM
dblaikie added inline comments.Nov 19 2021, 9:24 AM
llvm/test/tools/llvm-diff/phinode.ll
2

I'm not super faimiliar with llvm-diff - but I'm guessing this test passes before/after the change, so isn't testing the new functionality, is it? I guess some negative test cases should be tested instead to demonstrate that more parts of the IR are being tested?

void added inline comments.Nov 19 2021, 11:54 AM
llvm/test/tools/llvm-diff/phinode.ll
2

I was a bit dumbfounded that this tool didn't have "negative" tests (except for the callbr for some reason). This test is almost entirely a smoke test—can it run without barfing on a PHI node? I'll see about creating a negative test.

void updated this revision to Diff 388589.Nov 19 2021, 12:04 PM

Improve testcase to show an actual difference.

dblaikie accepted this revision.Nov 19 2021, 12:22 PM

Looks good - optional test change.

llvm/test/tools/llvm-diff/phinode.ll
2

To avoid all the escaping which maybe obscures the purpose of the replacement, maybe this could be s/ 0,/ 1,/ - yeah, means it /could/ match other parts of the IR, but it doesn't currently at least.

This revision is now accepted and ready to land.Nov 19 2021, 12:22 PM
This revision was automatically updated to reflect the committed changes.