User Details
- User Since
- Mar 24 2020, 8:40 AM (112 w, 6 d)
Mon, May 16
I am inclined to revert this change, in line with readability and the other targets. I commented above that I found the original structure clearer. Others said similarly in the changes for other targets. I was happy to bring XCore in line with other targets, but they have not gone for multiple operations on one line. So I would prefer to revert, but welcome comments.
Wed, May 4
Are you blocked in some way due to the mismatch?
This has changed test totals reported on the buildbot dashboard, so I would be interested in a comment on that.
Thu, Apr 28
XCore OK.
Mon, Apr 25
I see D123467 is the cross-target patch for this, and was accepted, so LGTM.
If D123656 gets consensus then this looks right to bring XCore in line with it. But personally I think the original structure is clearer: one operation per line, with the MVT and action right next to it. What is the motive for the change?
Mar 19 2022
LGTM thanks
Mar 18 2022
LGTM.
LGTM but I would prefer to hear from other reviewers who commented on object-emission before.
The code LGTM. When object-emission was reintroduced in D98508, there was some discussion on whether XCore ought to use this feature, or provide an integrated assembler instead. Sorry I don't know NVPTX: is NVPTX different because it generates assembly as its natural output, and binary does not make sense? Or could it have an integrated assembler at some point?
As the test is specifying targets, I think it needs to be in both X86 and XCore areas. Otherwise compilers not supporting those targets will attempt the test.
Mar 17 2022
LGTM
Jan 17 2022
XCore files LGTM. Nice improvement.
Sep 14 2021
Remove "unit" from description as recommended in review.
Sep 13 2021
Thanks @gkistanova . Yes, I will remove "unit" as suggested.
@ikudrin Thanks for the comment and the buildbot links. Yes, I won't check in till approved by buildbot maintainers.
Jul 1 2021
Jun 24 2021
The clang-xcore build status changed from "Unexpected test result output SKIPPED" in https://lab.llvm.org/staging/#/builders/145/builds/1340, to "X skipped unit tests" in https://lab.llvm.org/staging/#/builders/145/builds/1342. If that was when the patch was staged, it has worked on the display of results.
Jun 18 2021
Jun 16 2021
llvm/include/llvm/Target/TargetSelectionDAG.td defines SDTMemBarrier. This seems to have been introduced in 9b254eed32028 for ISD::MEMBARRIER, which was removed some time ago. I can't find a use of it and I can build without it. Could SDTMemBarrier be removed and would it belong in this patch?
XCore change LGTM. (Commenting on XCore only.)
Jun 14 2021
Yes, I will look at this for the XCore target.
Apr 29 2021
Currently the only memory operands that XCore inline asm supports are indirect (*m) and they have to be globals. It doesn’t handle locals as operands, or plain “m”. (It doesn’t handle FrameIndex.) So in that sense these tests are expected to fail. As an alternative to XFAIL, I’ve started looking at handling these other cases, or at least finding out what is difficult about them. I suspect there is some difficulty, otherwise the original developer would have done it, but I can’t say what it is yet.
Apr 28 2021
I'm looking at extending XCore inline asm, or at least understanding why it's a problem to handle these cases, to provide more context for any XFAIL.
Apr 26 2021
Apr 21 2021
@echristo Thanks for the comment, Eric. I'm happy to do what is expected. Do you mean that it's a bug not to accept these cases, and we should fix it rather than XFAIL? So the build should not be green from XFAIL but only from a fix in this case?
Apr 20 2021
In a quick test, I used else if (TI.getTypeWidth(TI.getWCharType()) == 8) but did you want to avoid that?
At the moment Clang defines __XS1B__ for XCore, but really that's one subtarget, I need to patch to define __xcore__ . It's an embedded processor, usually bare metal, so wouldn't have an OS character set. I just saw this change and thought I'd mention it's only 8 bits wide - I don't know what the best approach is. I'll discuss with others. Thanks for the quick reply.
The XCore target has 8-bit wchar_t (unsigned char). Should there be a test for width == 8 and set to "UTF-8", or at least not "UTF-16"?
Use XFAIL
Invite inline assembly code owner to review.
Apr 19 2021
Tidy up.
Apr 16 2021
Thank you for the reviews, I appreciate people taking time on something that is only for this target.
Apr 15 2021
Invite debug info code owner to review.
Apr 12 2021
As suggested, excluded xcore from DebugInfo/Generic rather than patching each file.
Apr 1 2021
Tidy up.
Omit DebugInfo/Generic on XCore to avoid annotating 70 separate files.
Mar 31 2021
Thanks, I see what you mean about churn and maintenance. The integrated assembler is planned. I will rework the patch.
DebugInfo/Generic can be handled by two lines in lit.local.cfg:
@MaskRay Thanks for the comment. I will look at that. But DebugInfo/Generic has 78 files requiring object-emission and 60 not requiring it - isn't it worth still running the 60? (I looked at some to see what they were doing: running llvm-as, llvm-dis, and checking assembly output.)
Mar 30 2021
Invite reviewers around llvm-mc tool tests.
Invite reviewers around llvm-mc tool tests.
Invite reviewers around lit.cfg.py changes.
Mar 24 2021
Mar 23 2021
Mar 22 2021
Mar 19 2021
Mar 18 2021
Add reviewer suggestions from CodeGen/Generic history.
Mar 13 2021
Thanks, Galina.
Mar 12 2021
Mar 11 2021
Thanks @clayborg
- Incorporate isConfigurationSupported in isObjectEmissionSupported
@labath Thanks for the quick response. That looks like a good idea. I will try that.
Run clang-format
Mar 10 2021
Mar 8 2021
arc --update with all changes.
[XCore] Do not override LLVM_LIT_ARGS: use -j from the factory.
Mar 4 2021
Mar 3 2021
Revise as per review comments.
Feb 23 2021
Jan 14 2021
Jan 9 2021
I think this patch can be cancelled, because the issue is already fixed in the main branch. But please say if I have missed something.
Jan 5 2021
I've reproduced the failure. The test passes when I change -march=xcore to -mtriple=xcore-unknown-unknown, as in main branch commit https://github.com/llvm/llvm-project/commit/15ca54525d6c2927b2a51b871a9e343c7ce1c2ea. Does that commit solve this problem and unblock D91556 ? (I will continue to investigate to understand the reason for the register swap.)
Jan 4 2021
The specified XCore backend code owner has moved on from XMOS and from working on XCore. I will have a look at this patch.
Dec 8 2020
Many thanks for review and approval. Please could it be committed as I do not have commit access? (I will request.)
Added reviewer suggestions from git history. If someone familiar with updating the documentation could commit for me, I would be grateful, as I do not have commit access.
Nov 25 2020
Run clang-format
Run clang-format
Alternatively, if all 1-byte-character strings are acceptable, should I remove the check that the first token is a string literal, and omit the failing test case on XCore?