- User Since
- Oct 24 2016, 8:15 AM (180 w, 1 d)
Can you make this dependent on https://reviews.llvm.org/D76902and rebase/repost.
- rebase to pick up landed dependencies.
- Moved assertion that loop index was still valid to top of loop.
- added a test that exercise fatal error for args split between registers and stack.
Which I think mishandles negative values.
run clang-format, but otherwise for fixing the assertion it LGTM. FWIW I'm not sure the code we produce for the conversion handles negative numbers correctly. For the reduced IR I posted I get:
Thanks for the update, LGTM.
Fri, Apr 3
I would rather we don't rename any of the ViaCopy lists, or CSR_SRV464_TLS_PE_SaveList since they are likely dead. We can write IR that triggers them, but Clang will only generate said IR for Darwin IIUC and from the name of the calling convention it is limited to C++.
Also please clang format the changes.
Thu, Apr 2
Wed, Apr 1
LGTM. Thanks for the updates Chris.
I suggest adding a test along the lines of:
Change looks good, however there is a couple things complicating this.
- The SplitCSR code is dead with Darwins removal and we didn't realize it.
- CSR_AIX64 and CSR_SVR464 are the same and should be one list, not 2 distinct lists.
Tue, Mar 31
1 comment, but otherwise LGTM.
Add a test that checks for a fatal error when a ByVal argument is split across registers and stack.
Mon, Mar 30
- Fixed wording on a fatal error.
- Changed the args and captures on the lambda.
- Restructured the loop as suggested.
- Added comment about the dead-stores in the lit test.
Fri, Mar 27
One comment, but otherwise LGTM.
Thank you for the updates. One minor comment, but patch LGTM.
Thu, Mar 26
A couple minor suggestions. If you agree with them, feel free to land them as an NFC update to the test before commiting this patch.
Wed, Mar 25
Tue, Mar 24
Added a comment to describe why we must store the registers used to pas a ByVal to the stack.
- split the ByVal handling into memory and register parts to remove an 'else'.
- add back some missing info for fixed stack objects in the lit test.
- remove check lines related to fixed stack objects which are not semantically meaningful.
Mon, Mar 23
- Fixed assertion with zero sized byvals that new testing exposed.
- Addressed various review comments (renamed TrueSize, use alignTo, etc).
Fri, Mar 20
I'm going to implement the suggested changes to the testing (ie a zero-sized-byval and different byval types so we are loading more then just a char). I'll do so in a separate patch which makes the changes and shows the difference in the existing tests and then I'll rebase this patch onto that.
- rename local to match other uses in function.
- constify a handful of locals.
Thu, Mar 19
Landed check-prefix changes as an NFC patch and rebased ontop of that.
Wed, Mar 18
Tue, Mar 17
Mon, Mar 16
As a first step, I suggest we break the clang changes and the LLVM changes into 2 separate patches.
Thu, Mar 12
Wed, Mar 11
Mon, Mar 9
Mar 6 2020
1 minor comment but otherwise LGTM.
Mar 5 2020
I get why you named the test ppc32-setcc-expansion.ll but to a casual reader its definitely unclear what a setcc has to do with this code. I would suggest 'ppc32-i64-to-float-conv.ll', and have a comment explaining how the lowering for the conversion generates a setcc which caused a crash under certain conditions.
Mar 4 2020
Reworded comment and added 64-bit RUN step to the test.
Please fix the formatting issues flagged by the pre-merge checks.
Mar 3 2020
Feb 26 2020
Feb 25 2020
Feb 24 2020
Feb 21 2020
Feb 20 2020
Fix the formatting that Lint called out, but otherwise LGTM for fixing building natively on AIX.
Fix formatting of CHECK lines in several of the lit tests.
Feb 19 2020
Patch LGTM as far a formatting/naming/testing etc. C++ specifics is outside my wheelhouse though, so I can't confirm things like the tail padding rules are correct for AIX. Because of that I'm not comfortable being the one to accept the patch.
- Added a lambda for building the instruction which moves the condition register into a scratch register to avoid dupication.
- Added an IR --> assembly test that verifies we restore all 3 non-volatile cr-fields.
- Removed a couple stale comments, and reworded another comment.
Feb 18 2020
Looks like there are calls to assembleToStream which didn't get updated to check that the return value is valid, breaking bots that treat warnings as errors: http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/1389
Feb 14 2020
- We should probably update the COMDAT section of the lang ref to mention XCOFF doens't support COMDATS.
- Will we report an error somewhere in the backend if we encounter a COMDAT?