In D75018#1889213, @arsenm wrote:

Testcase? Also typo in message

Just to note there is related commit https://reviews.llvm.org/rL366557.

This is already accepted. Do you want me to commit this on behalf of you?

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.

Ping!

In D63045#1535246, @jdoerfert wrote:

LGTM. Though I hope to remove the FunctionAttr code soonish.

Coge change looks good but I have few questions on lit test cases.

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

In D45308#1145575, @kbarton wrote:

@vivekvpandya You're right - the change will allow noCSROpt to run on functions with external linkage, which is not correct.
I think what we really want here is the isSafeForNoCSROpt to check for the existing conditions (i.e., prior to this patch, when it looks for local linkage) and also whether IPRA was successful for this function. I added the isDefinitionExact() call, to try and replicate the necessary conditions for IPRA, but now I'm wondering if it would be better to query the PRUI and make sure it contains a valid RegMask for the given function. Does this seem reasonable to you?

@kbarton can you please validate below tablel?

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.

In D45308#1083887, @jonpa wrote:

With this patch, any called function which is not "definition exact" keeps the unmodified regmask on the call instruction, which assumes that the callee saved registers are saved if modified by callee.

I therefore think the check !F.hasLocalLinkage() in isSafeForNoCSROpt() should be replaced with !F.isDefinitionExact(), since the former check only covers a subset of the second one.

Otherwise, the saving of the CSRs might be skipped without this being reflected in the call regmask in caller, right?

In D46232#1083113, @uweigand wrote:

But we do not want to force use of a frame pointer, in general this just reduces performance for no reason on Z. Some functions require a frame pointer (those that do dynamic allocas), but most do not.

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?

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.

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.

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.

In D44067#1042036, @fhahn wrote:

There are a few issues with formatting (trailing spaces, using tabs, lines > 80 chars, see https://llvm.org/docs/CodingStandards.html). Could you use clang-format to format the diff?

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?

Final looks good or rejection should be given by community member which have more experience with LoopVectorizer. Adding @nadav , @anemet.

Few high level comments from my side. For more accurate review I recommend wait until @anemet takes loot at it :)

updated

Updated patch to address @anemet 's comment.

changes Clang-formated!

Added method to detach unique_ptr<DiagnosticHandler> from LLVMContext.

update

Update.

Why? That was inside BackendConsumer.

Update

@anemet if this looks oky to you then I can commit this on weekend.

Test Updated

Clang-formated

In D33514#859615, @anemet wrote:

You also need to add a test. You should be able to extend either the LICM's or the Vectorizer's test to also get the remarks due to allowExtraAnalysis with -pass-remarks not just with -pass-remarks-output.

Updated the patch , tests will be updated in next patch.

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!

Update the patch.

Updated the patch but some cleanup is still to be done. For now I am focusing on correctness.

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

Ping!

ping!

In D32304#755329, @MatzeB wrote:

In D32304#754324, @vivekvpandya wrote:

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.

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

In D32304#754075, @nemanjai wrote:

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 don't have commit access. Kindly commit it. Sorry for the trouble.

ping!

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

Update the patch as per discussion.

In D32304#742192, @qcolombet wrote:

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.

Hi Matthias,

Ok I will update this for all optional mapping and then do testing on existing mir test files in various target folders.

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.

ping !

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.