I found that all values that have optional mapping will not be printed by YAML Output if its value is equal to default value.
So I have tried out this on MachineFrameInfo object and this works as per expectation.
It just prints 'frameInfo:' tag in YAML file with out any fields.
If we are agree on this I will update other objects mapping to have similar behavior.
I will update this patch so that if all properties of object is not printed then its name should not be printed as that causes MIR test failure in llvm-lit.
Details
Diff Detail
Event Timeline
Yes this looks like a good way to reduce the output and like a sensible first step.
I tried this patch and it obviously needs some test cases adjusted. It also seems that printing frameInfo: without any following fields is invalid YAML so that we probably need some fixes in YamlIO to leave out the complete frameInfo: header in case all fields have their default values.
(And for long term/later commits: Some properties like "hasStackMap", "hasPatchPoint", "hasCalls"... look like we should be able to just compute them instead of printing/reading them.)
As YAML will first process frameInfo object so it is not easy to remove "frameInfo" from output after finding it's empty. However if we provide default value for frameInfo mapping in MachineFunction class then we can eliminate it from output if entire frameInfo is having default value. So I have update the patch accordingly. If this approach seems good then I can send updated patch for all option mapping.
Ok I will update this for all optional mapping and then do testing on existing mir test files in various target folders.
Hi Matthias,
I have updated this patch to all optional mappings in MIR-YAML mapping. I have also updated X86 MIR test-cases which were failing as default values not getting printed. How ever there may be some code which is not really required. Also I will update the patch after testing it for other targets. But in meantime kindly look at changes and provide your feedbacks.
Hi guys,
I am not sure I like the approach.
I agree that as a human the among of MIR we need to write to make it work should be minimal. However, I believe the printer should be as dumb as possible. Printing is cheap and also, anytime the default changes, the printed output would still work.
I have the same reasoning for anything that can be computed from the input, like the live-ins, successors and so on.
The bottom line is I am in favor of a smart parser, but not a smart printer.
Cheers,
-Quentin
test/CodeGen/MIR/Generic/frame-info.mir | ||
---|---|---|
35–45 | FWIW, it could go away in the test without having it go away from the printed output. |
Hello @qcolombet
I am not sure I like the approach.
I agree that as a human the among of MIR we need to write to make it work should be minimal. However, I believe the printer should be as dumb as possible. Printing is cheap and also, anytime the default changes, the printed output would still work.
I am new to this thing but I would like to have my input from what ever I have understood till now.
I think not printing entities which have default value is useful when someone wants to write a new MIR test file by starting with llc generating a initial version, adding required check statements and other commands to it.
Also if someone prefers to have default values printed then we can provide a flag in llc that will map to WriteDefaultValues in YAMLTraits ( as llvm-pdbdump tool does it currently).
After having quick look at MIRParser I feel that with syntactically correct YAML it will create MachineFunction , entities which are missing from YAML input will have default value.
If default value changes then printer will just print it (as it may not be updated in MIRYAMLMapping.h ) but nothing will break.
I feel that challenges with default value will come when it is not possible to get default value for some target specific entity.
Sincerely,
Vivek
I have the same reasoning for anything that can be computed from the input, like the live-ins, successors and so on.
The bottom line is I am in favor of a smart parser, but not a smart printer.
Cheers,
-Quentin
-simplify-mir is not true by default so there is no need to change mir test cases which were modified previously.
I think it's time that you ask Chris for it as you have a few patches in by now (see http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access )
This appears to have broken MANY buildbots due to some compilation error that I haven't really looked at. I'll wait a bit longer to see if you (@vivekvpandya) pull it out/fix it and if it doesn't happen soon, I'll probably just pull it out.
I have reverted it on my SVN. Should I commit again with message "Revert my revisionID" to reflect that on llvm.org/svn ?
In previous patch constant used to set default value for object property of type int64_t , uint64_t was not portable.
Due to
if (!SimplifyMIR) Out.setWriteDefaultValues(true);
some test cases need to be updated. Sorry previously I didn't run llvm-lit on whole test folder.
Please provide reviews.
-Vivek
Also is there any way to test patch on build bot with out committing changes specially on different hardware?
No, we don't have infrastructure for pre-commit testing. It usually comes down to:
- Ensure that your patches are correct before committing. This of course can only be done up to some reasonable level, this usually means running ninja check-llvm if your patch is more complicated or touches unusual corner cases then it may be a good idea to think about what else you could test.
- Watch the buildbot emails coming back. It is sometimes tricky to figure out if it was really your patch that broke it. If in doubt talk with the other people on the blamelist, check the bot history for similar failures in case of flaky tests, ...
- If you believe your commit may be responsible: If you have a safe easy fix that you can apply within a short time (I usually aim for no longer than an hour) then go for that. In all other cases revert your commit! The goal here must be to get the bot back to green as fast as possible to avoid other errors going by unnoticed because the bot was red anyway.
I don't understand this explanation. Can you give some example? It feels odd that you had to add -simplify-mir to all of those tests...
If we make YAML:IO to write default values it will print every things including empty strings, for example consider test/CodeGen/X86/virtual-registers*.ll in that if we don't use -simplify-mir it will generate - { reg: '%edi', virtual-reg: '' } which makes POST-RA-NEXT check failed. This is why I have added -simplify-mir where required.
However in test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll I think adding -simplify-mir is not correct because it also removes basic block printing and for that particular case it is required that only one successor is present after certain statement so for that case I need to add fields which were not getting printed before but now they are getting printed.
Also before this patch we already do not print string with empty value so all these test-cases have been written in such manner that check for filed with empty string is not done so to preserve that I added -simplify-mir. Or we need to other way around.
I will keep your suggestions related to testing and committing in mind.
If we make YAML:IO to write default values it will print every things including empty strings, for example consider test/CodeGen/X86/virtual-registers*.ll in that if we don't use -simplify-mir it will generate - { reg: '%edi', virtual-reg: '' } which makes POST-RA-NEXT check failed. This is why I have added -simplify-mir where required.
However in test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll I think adding -simplify-mir is not correct because it also removes basic block printing and for that particular case it is required that only one successor is present after certain statement so for that case I need to add fields which were not getting printed before but now they are getting printed.
Also before this patch we already do not print string with empty value so all these test-cases have been written in such manner that check for filed with empty string is not done so to preserve that I added -simplify-mir. Or we need to other way around.
Argh that is unfortunate. However after settling on printing everything by default I think the conclusion here is to rather adapt the CHECK: statements in the tests than switching a whole lot of tests to a non-default output mode. Of course we should keep some test with -simplify-mir around to make sure that mode works.
I will keep your suggestions related to testing and committing in mind.
Sure, we are all breaking the build from time to time. Good that you reverted in time.
@MatzeB sorry for the delay I was on vacation. I have updated the patch as per your suggestion. Due to GlobalIsel related work some test cases have some updates frequently so it is better to close this as earliest as possible.
This can be const. Same thing for several of the following operator== implementations.