This is an archive of the discontinued LLVM Phabricator instance.

Make lazyload_metadata.ll resilient to the addition of new metadata kinds
ClosedPublic

Authored by herzka on Dec 19 2019, 3:33 PM.

Details

Summary

The specific number of records loaded depends on the number of kinds, but the difference between the lazy and not lazy cases does not.

Event Timeline

herzka created this revision.Dec 19 2019, 3:33 PM
modocache added a subscriber: ostannard.

As a maintainer of a downstream fork of LLVM with its own metadata kinds, I'm a big fan of this change. Until this patch:

  1. Whenever anyone added a metadata kind, either to upstream LLVM or to my downstream fork, they'd need to remember to increment the number in the check for 74 bitcode-reader - Number of Metadata records loaded.
  2. Whenever someone added a metadata kind to upstream LLVM and update this test, the upstream change to this test file would conflict with my downstream changes to the test file. Or, if there was no conflict because the number of records was the same, my downstream test would start failing, and I would need to open this test file and increment the count to account for my downstream metadata kinds.

This change makes this test less brittle and has it focus only on testing lazy loading, which is I believe what the original test author, @mehdi_amini, intended. (I'll also suggest @ostannard as a reviewer since he last incremented the count in this file.)

Anyway this looks great to me but I'll wait for more reviews.

wenlei added a subscriber: wenlei.Dec 20 2019, 7:25 AM
mehdi_amini added inline comments.Dec 20 2019, 10:50 PM
llvm/test/ThinLTO/X86/lazyload_metadata.ll
14

Never saw this syntax with cat and the two redirections before. Is this equivalent to the following?

; RUN: (llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc -stats 2>&1  | awk '{print "LAZY: " $0}' \
             && llvm-lto -thinlto-action=import %t2.bc -thinlto-index=%t3.bc -stats -disable-ondemand-mds-loading 2>&1  awk '{print "NOTLAZY: " $0}')
; RUN:       | FileCheck %s
mehdi_amini accepted this revision.Dec 20 2019, 10:50 PM
This revision is now accepted and ready to land.Dec 20 2019, 10:50 PM
herzka updated this revision to Diff 235187.Dec 23 2019, 5:37 PM
herzka marked an inline comment as done.

use subshell instead of cat to pipe both commands to FileCheck

herzka marked an inline comment as done.Dec 23 2019, 5:38 PM
herzka added inline comments.
llvm/test/ThinLTO/X86/lazyload_metadata.ll
14

Oops, yep. That's a lot nicer, thanks!

Do you need someone to land this for you?

This revision was automatically updated to reflect the committed changes.

Reverted because this broke the test on Windows (http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/21273/steps/test-check-all/logs/stdio). Is there any way I can run buildbot on a diff that's still in review? I'd like to debug this, but I don't have a Windows setup.