This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Split up huge aarch64-cpus.c test.
ClosedPublic

Authored by fhahn on Mar 3 2022, 12:57 AM.

Details

Summary

This test file has grown to the point where it takes a huge amount of
time to run. At the moment, this test seems to consistently time out
when running in the pre-commit checks in Phabricator with a 10 minute
timeout. For example see
https://reviews.llvm.org/harbormaster/unit/view/2832723/

While splitting up the test file is not ideal, it is even more
undesirable to have huge test files that time out in common settings.

This patch splits up the test file roughly in the middle.

Diff Detail

Event Timeline

fhahn created this revision.Mar 3 2022, 12:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 12:57 AM
fhahn requested review of this revision.Mar 3 2022, 12:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 12:57 AM
tyb0807 added a comment.EditedMar 3 2022, 1:26 AM

Thanks for making this change. However, I think a better naming would be

  • aarch64-cpus.c which contains cpu-specific test cases (usually with -mcpu flag). Roughly this is until line 515 (as you are doing), plus some last test cases from line 926 to end.
  • aarch64-archs.c which contains architecture-specific test cases (with -march flag).

Or better yet, we can further break down the aarch64-archs.c, with feature-specific test cases going into a feature-specific file, just as what we are doing with new features (aarch64-hbc.c, aarch64-mops.c, ...). I can submit a separate patch for this if you prefer.

While splitting up the test file is not ideal [...]

Actually I'm not so sure. I'd almost rather go further, and split it up into lots of much smaller files, each with some kind of reasonable theme, like "all the basically v8.1-A stuff" or "all the v8M".

The advantage of that is that it sets an obvious pattern for the next developer to follow – they look at the existing tests, notice there are lots of separate files, and make a fresh one rather than extending an existing one. Doing it this way, all that will happen is that one of the -1 and -2 files gets too big again in a year or two's time :-)

fhahn added a comment.Mar 3 2022, 1:39 AM

While splitting up the test file is not ideal [...]

Actually I'm not so sure. I'd almost rather go further, and split it up into lots of much smaller files, each with some kind of reasonable theme, like "all the basically v8.1-A stuff" or "all the v8M".

The advantage of that is that it sets an obvious pattern for the next developer to follow – they look at the existing tests, notice there are lots of separate files, and make a fresh one rather than extending an existing one. Doing it this way, all that will happen is that one of the -1 and -2 files gets too big again in a year or two's time :-)

Agreed, more fine-grained splitting would be ideal, but unfortunately I am not familiar enough with all the intricacies of the test to perform more fine-grained splitting.

dmgreen accepted this revision.Mar 3 2022, 11:58 PM

OK Cool. Lets not punish Florian for improving things. If anyone want to go and split this file sensibly, that sounds good. In the meantime this LGTM.

This revision is now accepted and ready to land.Mar 3 2022, 11:58 PM
fhahn updated this revision to Diff 412971.Mar 4 2022, 3:23 AM

Thanks everyone! I added a TODO for splitting up by category and plan to land this soon.
The update also adds a missing GENERIC check line.

This revision was landed with ongoing or failed builds.Mar 4 2022, 3:27 AM
This revision was automatically updated to reflect the committed changes.