This is an archive of the discontinued LLVM Phabricator instance.

[clang][RISCV][NFC] Combine RV32/RV64 ABI tests into single files

Authored by asb on Dec 20 2022, 7:23 AM.



After D134050, it makes sense to combine the RV64 ABI tests into a single file in order to make it more maintainable (i.e. not having to split tests based on the combinations of ABIs they're expected to impact). This patch deletes duplicated tests but doesn't do much further reorganisation beyond that.

I imagine the logical ordering of tests in the file and comments could be further improved in the future. My personal feeling is that it's probably not worth investing the time to try to get this "perfect", and to instead settle for this incremental step forward. But if there's reviewer interest in attempting to further iterate, I'm happy to do so.

Diff Detail

Event Timeline

asb created this revision.Dec 20 2022, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2022, 7:23 AM
asb requested review of this revision.Dec 20 2022, 7:23 AM

This looks good to me, but maybe wait a few days to see if anyone else has opinions

kito-cheng accepted this revision.Dec 20 2022, 5:46 PM

LGTM, I like this, this making adding ABI test easier in future.

This revision is now accepted and ready to land.Dec 20 2022, 5:46 PM
craig.topper accepted this revision.Dec 20 2022, 5:47 PM
asb added a comment.Dec 21 2022, 1:24 AM

This looks good to me, but maybe wait a few days to see if anyone else has opinions

Yeah, I'm blocked on the related patch review at the moment so this is unlikely to land particularly quickly.

asb updated this revision to Diff 501470.Mar 1 2023, 5:17 AM


MaskRay added inline comments.Mar 1 2023, 11:36 PM

These ^//$ lines immediately before the code should be removed.

asb added inline comments.Mar 8 2023, 2:18 AM

It would be nice to do so, but that's what the tool currently outputs. Given the back and forth and iterations needed to have --function-signature include the return type, I've already invested quite a bit of time into and it's hard to justify much more for a cosmetic issue.

Given I think this change is still overall positive for maintainability despite the fact the output could be cleaner, would you be happy for me to proceed with this and to file an issue for further improvements?

asb added inline comments.Mar 14 2023, 5:45 AM

Hi @MaskRay - there are a few patches lined up depending on this one so I just wanted to check again if you're happy with the path forwards proposed above (i.e. to land as-is and to leave any further iteration on producing nicer output for future work)?

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2023, 10:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript