User Details
- User Since
- Jul 30 2015, 11:44 AM (356 w, 1 d)
Feb 24 2020
Feb 23 2020
Aug 24 2019
Jul 19 2019
Just to note there is related commit https://reviews.llvm.org/rL366557.
Jul 15 2019
This is already accepted. Do you want me to commit this on behalf of you?
Jun 23 2019
Minor suggestion, you may want to update http://clang.llvm.org/cxx_status.html page for "typename optional in more contexts" with in this change.
Jun 19 2019
Jun 18 2019
Ping!
Jun 15 2019
Jun 9 2019
Jun 8 2019
Jul 24 2018
Coge change looks good but I have few questions on lit test cases.
- in llvm/test/CodeGen/PowerPC/ipra-odr.ll IPRA is not called on bar() then it should follow calling convention so in foo() why registers are getting restored between calls to bar() because without IPRA bar() is expected to save callee-saved-registers as per ABI.
- llvm/test/CodeGen/X86/ipra-external-linkage.ll same question as above applies. As foo() will save callee-saved-registers as per ABI why around foo() call in bar() it should save/restore callee-saved-register.
Jul 3 2018
Jun 10 2018
@kbarton can you please validate below tablel?
May 1 2018
I don't understand if this change address any correctness issue. If this in response to recent test failures you discovered for SystemZ then I think those tests are failing because SystemZ::determineCalleeSaves() did not do job as per expectation. Also your proposed change uses getCalleeSavedRegs() and it adds a loop however previous change is using getCallPreservedMask() which is faster.
It is better to get this reviewed from more experience person in register allocation.
Apr 30 2018
I run ipra-02.ll with --enable-ipra option with lldb the contorl reaches to SystemZFrameLowering::determineCalleeSaves() through https://github.com/llvm-mirror/llvm/blob/5b508199fa870516e29b847b8af2f85887e05d56/lib/CodeGen/PrologEpilogInserter.cpp#L525
for both functions HasFP is set to false that is why determineCalleeSaves() does not add R11. Can you please verify this?
Apr 29 2018
When enabling IPRA on SystemZ, a benchmark crashed and I found out that a function had been optimized to not save the Callee Saved Registers, which unfortunately included the return register (%r14) on SystemZ, so when the function tried to return UB resulted.
Apr 18 2018
Change looks good to me. But please fix the new test case as suggested by @nemanjai
In this new test do you see improvement in register allocation due to IPRA? If test-case does not show any benefit due to IPRA then it is not suitable test for this patch.
Also I hope this patch is also tested with other architecture which have LIT based test-cases for IPRA. It is very unlikely that such test-case will have function with linkage type which return true for isInterposableLinkage() but if such case is there then this change may break such tests.
Apr 7 2018
I am not an expert for InstCombine. But I think @spatel has worked on it for long time so I think he is appropriate person to review this.
Mar 19 2018
Mar 17 2018
In first pass of the review I did not pay attention to logic in this change, but I think this change is not required because as per my understanding from the code the best cost of vectorization is already calculated by the code, when vectorization is not beneficial it just not generating optimization remark, but if when vectorization is forced by user it does print this information in debug output statements. I am referring DEBUG(if (ForceVectorization && Width > 1 && Cost >= ScalarCost).
Is there any bug reported for this? if yes then please mention that.
Well if there is no bug reported for this then do you have any motivating example or other community member has suggested this improvement?
Mar 16 2018
Few high level comments from my side. For more accurate review I recommend wait until @anemet takes loot at it :)
Oct 11 2017
Oct 10 2017
updated
Oct 8 2017
Updated patch to address @anemet 's comment.
Oct 3 2017
changes Clang-formated!
Sep 26 2017
Sep 15 2017
Sep 13 2017
Added method to detach unique_ptr<DiagnosticHandler> from LLVMContext.
update
Sep 12 2017
Update.
Why? That was inside BackendConsumer.
Sep 11 2017
Update
Sep 8 2017
@anemet if this looks oky to you then I can commit this on weekend.
Sep 6 2017
Test Updated
Sep 5 2017
Clang-formated
Updated the patch , tests will be updated in next patch.
Aug 31 2017
Updated the patch! I don't think now we require to save OldDiagnosticHandler and context as we have ClangDiagnosticHandler class with creation of CreateASTConsumer, but I need some review.
Updated the patch!
Aug 27 2017
Update the patch.
Jul 31 2017
Jul 26 2017
Updated the patch but some cleanup is still to be done. For now I am focusing on correctness.
Jun 6 2017
Jun 5 2017
@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.
May 24 2017
Ping!
May 18 2017
ping!
May 15 2017
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.
May 14 2017
Also is there any way to test patch on build bot with out committing changes specially on different hardware?
In previous patch constant used to set default value for object property of type int64_t , uint64_t was not portable.
Due to
May 13 2017
May 11 2017
I don't have commit access. Kindly commit it. Sorry for the trouble.
ping!
May 6 2017
-simplify-mir is not true by default so there is no need to change mir test cases which were modified previously.
May 3 2017
Update the patch as per discussion.
May 1 2017
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.
Apr 28 2017
Hi Matthias,
Apr 27 2017
Ok I will update this for all optional mapping and then do testing on existing mir test files in various target folders.
Apr 25 2017
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.
Apr 21 2017
Apr 20 2017
Oct 18 2016
Sep 2 2016
Aug 28 2016
Jul 20 2016
Sorry @MatzeB I don't have commit access. Usually @mehdi_amini commits for me. If you are not able to find time I guess @mehdi_amini will do before this week ends.