- User Since
- Jan 4 2017, 12:12 PM (197 w, 5 d)
Sat, Oct 17
Fri, Oct 16
Even if these were implemented on RISC-V they'd just be stubs anyway because there is no support for hardware FPU traps, you have to check fflags in software.
Thu, Oct 15
As for building correctly:
(so please close this revision now and follow the right process in future)
As I said in D88832:
Tue, Oct 13
Why in 2020 are we *adding* new backwards compatibility aliases? RISC-V is young enough that we don't need to do this, just fix the code. QEMU has even dropped support for 1.9 already.
Try now; depending on instance configuration it sometimes only lets you close accepted revisions (based on the assumption that the commit shouldn't have landed unless accepted).
It was never closed, only accepted.
(and please also close the revision)
Please use arc patch next time to get the Differential Revision: header so the commit and the revision are linked in both directions, which will also automatically close the revision.
Mon, Oct 12
Thu, Oct 8
Wed, Oct 7
But yes, this whole time I've wanted a StringRef function and an ArrayRef<StringRef> function, with the former wrapping the latter.
Tue, Oct 6
Use an ArrayRef for the argument and put the one-class overload in a header so it can be inlined?
Mon, Oct 5
Also please update your review with a full context diff.
Why not make it general and take an array?
Tue, Sep 29
Why M680x0? Everyone knows it as m68k or 68k and that's what's in the triple, so surely M68K would be the logical target choice?
Thu, Sep 24
Tue, Sep 22
Can we please just have a riscv version that works for both RV32 and RV64? It should just be a case of changing some ld/sd's to LOAD/STORE macros that are either [ls]w or [ls]d, and the various multiples of 8 to n*__riscv_xlen instead.
Sep 18 2020
Sep 17 2020
If someone cares about pthread_create they might wish to follow up on my https://reviews.llvm.org/D58531, which I filed early last year to permit pthread_create to have a proper type in the syntax. It will likely need rebasing, but probably isn't that much work.
(and FreeBSD has an __always_inline in sys/sys/cdef.s like __inline)
Yes I think everything's been addressed now (though if I keep looking over it I might start nit-picking even more :)).
This has significantly regressed FreeBSD's performance with the new version of Clang. It seems Clang does not inline functions at -O1, unlike GCC, and since FreeBSD currently compiles its kernel with -O whenever debug symbols are enabled (which, of course, is almost always true), this results in all its static inline helper functions not being inlined at all, a pattern that is common in the kernel, used for things like get_curthread and the atomics implementations.
Sep 16 2020
This is currently incompatible with the save/restore libcalls, and I don't think there's any way to avoid that (the restore libcall both loads ra and jumps to it). We should ensure combining them gives an error.
Sep 13 2020
Sep 7 2020
Isn't it useless without emitting a PT_OPENBSD_RANDOMIZE to cover it? (reading the somewhat terse https://github.com/openbsd/src/blob/master/libexec/ld.so/SPECS.randomdata)
Sep 6 2020
Needs a test case; something like:
Sep 5 2020
Use llvm-readelf rather than llvm-readobj
Sep 4 2020
Sep 1 2020
The documentation currently shows __capability being included, but from looking at this patch does the configuration file not append (which I think makes sense, at least for __capability) rather than replace?
Actually, __sparcv8 is only for V8; if you have 32-bit V9 on Solaris it defines __sparcv8plus _instead_:
Aug 31 2020
And notably _doesn't_ define the V8 macros, which this patch then reintroduces.
GCC on Linux defines __sparc_v9__ even with -m32. I don't know what Solaris does but please don't break other operating systems just because Solaris has broken headers that conflate the CPU and the ABI.
Aug 25 2020
Not so silly; gdb (well, the names are inherited from bfd) has set architecture riscv:rv32/set architecture riscv:rv64 :)
Aug 24 2020
I think ORI and XORI should go together, as they're both moves when the immediate is 0. ADDI with 0 is special as the canonical move instruction, whereas ORI/XORI with 0 are not necessarily moves in microarchitectures, so I don't know whether they should be recognised here or not.
Aug 20 2020
Fixed stray non-breaking spaces
Aug 11 2020
Aug 6 2020
If changes like this are required for all these tests this is going to be a complete pain for downstream forks like ours. At the very least, make whatever script you used to update these public, as I don't want to have to recreate it from scratch when merging this in. I had enough "fun" with the LLD mass-renaming (UpperCamel -> lowerCamel) and that was _with_ a supposedly-working script (it didn't quite do the right thing and I seem to recall there being two refactoring commits, only one of which had a script); I do not want a repeat of that experience.
Subject to the tests all passing, I think this is now good.
Have you seen this actually occur? The fact that hasSideEffects is set surely means that it cannot possibly be duplicated, nor scheduled out-of-order with other side-effect-affected instructions? Also, the only CSR instructions that currently get generated _by CodeGen_ are reads of cycle[h], which are very well-behaved. Naive implementations might still take a slow path just because they're CSR instructions, but it'd be very easy microarchitecturally to quickly read the cycle counter in an ALU as if it were just a normal GPR move. In such cases you really would want to be able to express scheduling information for it just like other arithmetic operations.
Aug 5 2020
Looks fine, other than please mention something about it being an alias in the commit subject when landing as currently it reads as if it's a new CSR (and I'd also personally change "This patch enables the use of mucounteren." to "This patch enables the use of the old mucounteren name." to make it completely clear, but if the subject is fixed then I don't think it's _necessary_).
Aug 2 2020
Patch looks good but please fix the title and description of this revision. It's not about adding AltName, it's about adding mountinhibit itself, and as part of that you're including support for the older name too.
Aug 1 2020
Actually the code in the backtrace points at D82651, not this. I did however see buildAnyextOrCopy in a backtrace when playing around to get that minimal case, which is what brought me to this revision. Investigating further, it seems that this is because of D82651 not fully implementing this case, and future revisions not fixing that. Prior to that revision, inline asm would hit reportTranslationError and then fall back to SelectionDAG due to isGlobalISelAbortEnabled being true (provided -global-isel wasn't passed to override that, as is done in my test, but not the original C), but now, since it claims to be implemented but not correctly, it instead asserts deep down and is unable to fall back to SelectionDAG.
I believe this has broken AArch64 (-global-isel not required due to defaults, but it's a GlobalISel bug so should force it nonetheless):
Jul 31 2020
Jul 30 2020
There is a possibly-compelling argument against using x18: RV32E only gives x0-x15, so would not be able to support the current implementation.
Given @luismarques's comment, have you now actually run the tests (and I mean all RISC-V tests, not just branch-relaxation.ll, in case anything has been missed)? Perhaps I shouldn't take it for granted that people run tests before submitting patches, including revisions (though I'd still verify myself before committing anything), but you really should as otherwise it just wastes our time having to tell you they're broken when you could just run them yourself.