This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] [Clang] Clean up arm_mve.td file
ClosedPublic

Authored by Paul-C-Anagnostopoulos on May 11 2021, 7:25 AM.

Details

Summary

This patch does a little cleanup on the arm_mve.td TableGen file.

Could someone give me suggestions on how to test this? It builds fine, but I'm not sure how to test it. Of course, I will make sure that the resulting .inc files don't change. Anything else?

Diff Detail

Unit TestsFailed

Event Timeline

Paul-C-Anagnostopoulos requested review of this revision.May 11 2021, 7:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 7:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Testing the inc files sounds good to me. There are also tests in places like clang/test/CodeGen/arm-mve-intrinsics that will test this, which seem to all be passing? Anything that does #include <arm_mve.h>

For this kind of pure refactoring on a .td file, my usual approach to testing it is to run the file through Tablegen without any output option (i.e. just in the default -print-records mode), before and after the change. In this case, I'd think, you ought to be able to expect bit-for-bit identical output – and if you get it, then you can be pretty confident that all the directly useful output modes won't have changed either.

For example, that's how I tested the refactoring in D72690.

craig.topper added inline comments.May 11 2021, 10:49 AM
clang/include/clang/Basic/arm_mve.td
579

Did you intend to leave the old lines commented out?

clang/include/clang/Basic/arm_mve.td
579

Just for the first review, in case anyone found anything crazy. I'll remove them when I update the review.

The arm_*.inc files do not change with this revision.

I removed the commented-out code.

One must amend the commit when one makes changes to the files.

simon_tatham added inline comments.May 12 2021, 6:59 AM
clang/include/clang/Basic/arm_mve.td
1532

If you've removed my unreadable foldl then you can also safely remove the comment in which I had to explain what it means :-)

clang/include/clang/Basic/arm_mve.td
1532

Will do!

Could I get an LGTM on this? I have removed the spurious comment.

jansvoboda11 accepted this revision.May 18 2021, 7:19 AM

LGTM provided the output file stays the same.

This revision is now accepted and ready to land.May 18 2021, 7:19 AM
dmgreen accepted this revision.May 19 2021, 1:17 AM

Sorry. I think we were waiting for the foldl comment to be removed. If you remove that, this LGTM.

Sorry, I removed it but didn't update this review. All set now.

This revision was landed with ongoing or failed builds.May 20 2021, 6:40 AM
This revision was automatically updated to reflect the committed changes.