User Details
- User Since
- Jun 17 2019, 7:18 AM (83 w, 6 d)
Tue, Jan 19
Minor suggestion to keep and remap some of the diagnostic cases.
Sep 23 2020
Reworked the prefixes and indentation.
Sep 22 2020
Added ppc64le target.
Jul 29 2020
Jul 22 2020
Jul 21 2020
added some commentary notes for the review.
Modified the tests to work at opt. Replaced initialization of return values with literals to simplify the IR.
Jul 20 2020
Formatted the RUN steps.
I'll format the RUN steps as suggested but with respect to the other suggestions, I prefer to keep the test as-is.
Jul 17 2020
May 8 2020
Apr 20 2020
Apr 16 2020
I think the review title "ByVal formal argument support: passing on the stack" implies that it covers the reg/stack split case. I noted you described it better in the review description. I think a slightly more descriptive abstract would be useful for the commit message.
Apr 15 2020
Apr 14 2020
Added MIR expect output for tests that expect calls to memcpy to ensure the memcpy calls are generated independently from caller register setup.
Apr 13 2020
Addressed unnecessary iterator check while looping on expect byval argument registers.
Apr 12 2020
Rebased to account for changes committed in D76902.
Apr 7 2020
My comments are only suggestions/discussion and can be addressed as you see fit on the commit.
I had intended to move the 5-8 byte byval callee cases out of aix64-cc-byval.ll and put them here. I don't want aix64-cc-byval.ll to exist when we're done. If the 5-8 byte tests aren't useful for the callee I'd rather delete them. They were of interest to the caller handling to make sure that we implement the left justification correctly.
Apr 6 2020
Apr 2 2020
Fixed expected output for 64-byte byval arg test.
Apr 1 2020
Added a test for the homogeneous float stuct Sean suggested.
Mar 31 2020
Tweaked the lambda capture on GetLoad.
Discussion ensued post-approval regarding use of CHECK-DAG on the ADJSTACKDOWN lines so I thought I'd post the final version before committing.
Mar 29 2020
Mar 27 2020
Clang format.
Mar 26 2020
Forgot the 64 byte byval test.
I split out some of the suggested test improvements to https://reviews.llvm.org/D76875
Mar 24 2020
Mar 23 2020
Summary:
-Test tidy ups
-FYI that I noted a 32-bit 8 byte aligned by-val (struct S { double d; }) that will need special handling in future
-Requested some comments in the code / tests to explain the suboptimal codegen, which otherwise looks odd.
Mar 20 2020
I still have to update the tests as per Sean's suggestion but I'll post the updated code for now.
Updated code to address comments.
Mar 18 2020
Mar 17 2020
Addressed Sean's comments:
-General test tidy up, remove an unnecessary assertion, tweaked a variable rename.
-Added skipping of 0 size by val args.
-Tidied up tests.
-Add CC_AIX logic to pass over 0 size by-val arguments.
-Tidy the by val tests added.
-Some expected output changes required in aix-cc-byval.ll related to use of ZEXTLOAD now that I've added the run step.
Added the missing run steps to aix-cc-byval.ll
Mar 16 2020
-Use ZEXTLOAD rather than EXTLOAD. This changed expected output because we're able to use rotate left word immediate with mask form instructions.
-Split byval cc tests into new test files.
- Split byval cc tests into a separate file.
- Use ZEXTLOAD
Mar 10 2020
Mar 9 2020
Mar 5 2020
One comment update. Should be ready to commit.
Mar 4 2020
Some of the prior test changes have been committed as NFC patches. I've updated the diff and readers can now see the instruction changes for widening arguments to register width before writing to the stack.
Feb 26 2020
I think we need some changes to ensure frame objects are created with their proper size and not word size. i64 is passed as two i32s but we should coalesce these into a single 8-byte object. Like-wise arguments smaller than word size are generating frame objects that are word size.
Feb 25 2020
Feb 24 2020
Feb 20 2020
Feb 19 2020
All minor comments except for the concern on whether or not a truncation node is needed when ValSize < LocSize.
Feb 18 2020
From my perspective, the only issue holding this up is settling on the name. I'd like to hammer that out and get this committed.
I believe the current patch is sound and correct. I had one test suggestion and one suggestion to use a lambda to reuse some of the cr spill logic.
Feb 13 2020
Code semantics look ok. If you can post a revision to address the minor concerns I need to have one more pass-through of the test updates in detail.
Feb 12 2020
I would like to see some test coverage for the AIX CRSave assembly, whether we adapt the tests you've already updated or add a separate test. It seems 64-bit PPC Linux has a better way of handling a single CR save. If we choose not to use the same approach on AIX I'd like to understand why.
Looks fine, but we need to settle on the name for the ABI. My preference would be "XLC++11", or perhaps "XLCXX11" (I propose the latter because of the common reference CXXABI.)
Feb 10 2020
This concludes my initial review. Mostly tidy up comments. The logic seems correct so semantically I think it's ok (for the incremental support that it adds.) I'll see if I can break it on the next round.
Did a passthrough of the test changes with a number of comments. I expect you wanted to make some test updates, nonetheless I thought I'd document what I'd like to see in the test coverage. Will review code changes next.
There's some trailing whitespace that emits warnings when I apply the patch. Probably a good idea to tidy it up. I'll continue to review the content of that patch.
Feb 5 2020
Used custom mem to tag memLocs for float args that also pass in register so the LowerFormalArguments_AIX can skip them easily.
Addressed Sean's comments. Notable change: when a float arg passes in FPR as well as PSA memory we will mark the memLoc custom. LowerFormalArguments_AIX can ignore custom memLoc. If a float arg passes in PSA memory and there was no FPR available, then the memLoc will be normal (non-custom.) I've updated LowerFormalArguments_AIX such that it will pass over custom memloc but continue to report_fatal_error on non-custom memloc. Zarko will have a patch coming to remove the report_fatal_error and implement memLoc for LowerFormalArguments_AIX.
Feb 3 2020
Only minor test questions / requests. The change looks ok. I think we should explain the stack adj differences with the XL compiler before committing, specifically: do we need to allocate the PSA for the current frame even if there are no calls?
Jan 29 2020
Some early issues. I need more time to go through the AIX assembly in detail to ensure it's correct.
Jan 28 2020
Added a test for a double that passes in fpr and 2 gpr.
Jan 27 2020
Jan 23 2020
Jan 22 2020
Jan 21 2020
Preliminary comments. I still need to go through the test changes more thoroughly.
Jan 10 2020
Already approved but I've addressed the nits and posted the final commit here because I'll need someone to commit it for me.
Jan 9 2020
Rewrote comments in aix-cc-altivec.ll test, fixed the vector diagnostic bug Zarko pointed out and addressed small nits.
Jan 8 2020
Jan 7 2020
I've decided to use custom regs for all gpr inits for float varargs. I discovered that f64 passing in 64-bit gpr was missing a required bitcast to int to handle the initialization. The new logic ensures the bitcast occurs for any custom handling. Custom handling is only expected for gpr inits of float args.
Dec 17 2019
Recent problems have come up generating the GPR initializations for floating point args, which may require some rework. Hold off on review until revision is posted.