Page MenuHomePhabricator

Reduce printing values with default in MIR based codegen testing YAML
ClosedPublic

Authored by vivekvpandya on Apr 20 2017, 11:14 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vivekvpandya created this revision.Apr 20 2017, 11:14 AM
vivekvpandya edited the summary of this revision. (Show Details)Apr 21 2017, 9:46 AM
MatzeB edited edge metadata.Apr 21 2017, 11:05 AM

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.)

vivekvpandya updated this revision to Diff 96604.EditedApr 25 2017, 11:03 AM

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.

vivekvpandya planned changes to this revision.Apr 25 2017, 11:04 AM
This comment was removed by vivekvpandya.
MatzeB accepted this revision.Apr 27 2017, 4:20 PM

LGTM, provided it passes the lit tests.

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.

This revision is now accepted and ready to land.Apr 28 2017, 12:42 PM
vivekvpandya requested review of this revision.Apr 28 2017, 12:42 PM
vivekvpandya edited edge metadata.
MatzeB added inline comments.Apr 28 2017, 12:49 PM
include/llvm/CodeGen/MIRYamlMapping.h
75 ↗(On Diff #97144)

This can be const. Same thing for several of the following operator== implementations.

test/CodeGen/MIR/Generic/frame-info.mir
29–30 ↗(On Diff #97144)

So nice to see this all go away :)

qcolombet requested changes to this revision.May 1 2017, 7:56 AM

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
29–30 ↗(On Diff #97144)

FWIW, it could go away in the test without having it go away from the printed output.

This revision now requires changes to proceed.May 1 2017, 7:56 AM

Hi guys,

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

vivekvpandya edited edge metadata.

Update the patch as per discussion.

vivekvpandya marked an inline comment as done.May 3 2017, 1:08 PM
qcolombet added inline comments.May 4 2017, 3:50 PM
lib/CodeGen/MIRPrintingPass.cpp
27 ↗(On Diff #97716)

@MatzeB already created an option in D31262. Sync your patch with that one.

-simplify-mir is not true by default so there is no need to change mir test cases which were modified previously.

This revision is now accepted and ready to land.May 11 2017, 10:48 AM

I don't have commit access. Kindly commit it. Sorry for the trouble.

I don't have commit access. Kindly commit it. Sorry for the trouble.

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 revision was automatically updated to reflect the committed changes.

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.

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 ?

vivekvpandya reopened this revision.May 13 2017, 4:30 AM
This revision is now accepted and ready to land.May 13 2017, 4:30 AM

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?

vivekvpandya requested review of this revision.May 15 2017, 7:28 AM
vivekvpandya edited edge metadata.

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.

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.

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...

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.

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.

MatzeB accepted this revision.Jun 5 2017, 4:32 PM

LGTM, thanks for doing all those fixups in the tests!

This revision is now accepted and ready to land.Jun 5 2017, 4:32 PM
Closed by commit rL304779: (authored by vivekvpandya). · Explain WhyJun 6 2017, 1:17 AM
This revision was automatically updated to reflect the committed changes.