-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ctx_prof] test tool: generate ctxprof bistream from json #100379
Conversation
This is a tool to simplify testing. It generates a valid contextual profile file from a json representation. The tool is authored to allow for future evolution, e.g. if we want to support profile merging or other tasks, not necessarily scoped to testing.
; EMPTY-NEXT: <Version op0=1/> | ||
; EMPTY-NEXT: </Metadata> | ||
|
||
; Note that uin64_t are printed as signed values by llvm-bcanalyzer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "uin64_t"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
return 0; | ||
} | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to emit an error if it isn't FromJSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flag mechanism takes care of that, added a test though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this should be llvm_unreachable then, or have an assert?
[ | ||
{ | ||
"Guid": 2000, | ||
"Counters": [4, 5] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these counter values in the tested output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only tested the values for a context, switching to testing the whole output.
}, | ||
{ | ||
"Guid": 18446744073709551612, | ||
"Counters": [5, 9, 10] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see these counter values in the tested output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
"Guid": 18446744073709551613, | ||
"Counters": [6, 7, 8] | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, can you align the matching brackets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed - the reference file is indented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is github that is making the spacing look off then. Because the end bracket here on line 16 looks to my eyes unaligned with the corresponding open bracket on line 7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh you meant the json not the xml output. sorry. Misread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used a formatter this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with a few suggestions
; Also we have no callee/context at index 0, 2 callsites for index 1, and one for | ||
; index 2. | ||
; RUN: llvm-bcanalyzer --dump %t/valid.bitstream | head -27 > %t.valid.in | ||
; RUN: diff %S/Inputs/valid.expected %t.valid.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the expected to be inlined here like it was earlier, which will be easier to analyze (and give more meaningful error output) on a mismatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
"Guid": 18446744073709551613, | ||
"Counters": [6, 7, 8] | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is github that is making the spacing look off then. Because the end bracket here on line 16 looks to my eyes unaligned with the corresponding open bracket on line 7?
} | ||
return 0; | ||
} | ||
return 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this should be llvm_unreachable then, or have an assert?
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/2313 Here is the relevant piece of the build log for the reference:
|
Follow up from PR llvm#100379, this is to avoid output formatting differences.
Follow up from PR #100379, this is to avoid output formatting differences.
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/979 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/1363 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/1343 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/1002 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/35/builds/1357 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/865 Here is the relevant piece of the build log for the reference:
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/161/builds/882 Here is the relevant piece of the build log for the reference:
|
|
||
static cl::SubCommand FromJSON("fromJSON", "Convert from json"); | ||
|
||
static cl::opt<std::string> InputFilename( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #89884 (comment) , cl:: is incompatible with GENERATE_DRIVER. Either remove GENERATE_DRIVER, or move this to Opt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, I'll take care of of it. Thanks.
Follow-up from PR llvm#100379 (llvm#100379 (comment))
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/3505 Here is the relevant piece of the build log for the reference:
|
This is a tool to simplify testing. It generates a valid contextual profile file from a json representation.
The tool is authored to allow for future evolution, e.g. if we want to support profile merging or other tasks, not necessarily scoped to testing.
Issue #89287