User Details
- User Since
- Jan 4 2017, 12:12 PM (210 w, 3 d)
Yesterday
Thu, Jan 14
Wed, Jan 13
Which GDB tests? How are they broken? I'm extremely surprised by the given this patch implements what binutils has done for a very long time (only .asciz/.string have changed behaviour in binutils, and those are not modified by this patch here so should not have regressions).
Tue, Jan 12
Please make sure there are tests for invalid instructions (especially checking you have the immediate ranges and predicates correct).
Having said that I do think it makes sense to push the getOS call into getOSABI by passing the full triple as it simplifies all the callers (and that also decouples the NFC refactor from your proposed behavioural change).
Looking at the original motivation for the patch I'm not convinced that llvm-readelf etc shouldn't just fall back on displaying the "normal" set of GNU extensions even for ELFOSABI_NONE (i.e. those that FreeBSD and GNU have in common). Things like that are really just part of de-facto generic ELF these days. What does binutils do?
Mon, Jan 11
Fri, Jan 8
🎉 (looks sensible to me but don't have a huge amount of experience with MIR)
Wed, Jan 6
Mon, Jan 4
One thing I don't understand is why RVV incoming/outgoing arguments need their own stack slots. I assume they're just passed indirectly, so why do we need to distinguish between arguments and locals rather than just treat them like anything else that ends up being passed indirectly?
Your tests look like copies of the F/D/Zfh tests with not all the comments updated and instances of tests that just don't make sense for Zfinx. I only skimmed them and picked up a few issues, I haven't gone through them thoroughly, please do that yourself.
Sun, Jan 3
Sat, Jan 2
Wed, Dec 30
I still don't understand what problem code beads are trying to solve that isn't already solved by existing backends like X86. Why can't you just assign operands to instruction encoding bits like a normal backend?
Wed, Dec 23
Tue, Dec 22
Mon, Dec 21
I agree with @lenary, I think this is fine to land (with the whitespace fix :)) and if it turns out the threshold is too low then we'll soon find out and can adjust accordingly (but I imagine not, 5 feels like a sensible default). Or if it's too high!
Doesn't this render fesetround useless?
Sun, Dec 20
I see very little by way of testing the return side of the calling convention, and nothing for sret.
Fri, Dec 18
Dec 17 2020
Looks good to me, let's see what the others think.
Dec 15 2020
Firstly, please generate your diffs with full context (-U with a sufficiently-large number). Secondly, can we avoid having to do a bunch of duplication with some clever use of multiclasses for F/D/Zfh and pseudos? Though maybe it's small enough that the duplication is easier to reason about than an obfuscated abstracted version.
Dec 12 2020
Dec 11 2020
Dec 10 2020
Dec 8 2020
i.e. can we not just support both approaches and prefer x86_64-linux-gnux32 if it exists?
Note that this shows there is currently another bug in x86_64 clang (ignoring OS, since with manual -march=athlon64 on macOS you can also get the non-macOS behaviour). Whether or not libatomic is used is part of the ABI, as libatomic may (and likely will for 16B atomics on your standard Linux distro) use hash tables of locks to emulate atomic operations, which will not correctly synchronise with the inlined hardware atomics. See https://godbolt.org/z/fqfvev for evidence of the ABI breakage.
Dec 7 2020
Dec 4 2020
Dec 3 2020
Ah, I hadn't realised the F comment got lost in 741b04b0b7912611a8a5b7e74462e87b8930a116, I was looking at our fork which is currently still based on 11. This looks good to me now.
Dec 2 2020
Yes that looks like everything now; thanks!
It's also wrong if the ADDI has FrameReg as its destination. A neater approach might be to always use the destination register (if there is one, using general instruction query information) as the destination for the ADD, keeping the scratch register for the movImm call. Then you'd only need special logic to work out whether you need to keep the (modified) existing instruction.
This is a bit ugly. Surely a simple peephole optimisation should be able to fix this and any other cases more generally? But also please give this a better title and commit message.
Dec 1 2020
I thought I'd gone through it thoroughly last time but apparently not :( oh well hopefully the last set of nits.
Nov 30 2020
Seems good other than additional comments regarding code clarity.
Couple of minor changes but otherwise LGTM.
Nov 29 2020
Nov 28 2020
Losing types and permitting nonsensical operators like !subst on code seems a bit sad. Why not just define a !codeconcat, like how we have a separate !listconcat? I agree that the TypeOf_xxx stuff is really ugly and so having _greater_ expressivity like !codeconcat would go a long way to help that. Also, cast<code>(str) would be the natural way to express the cases where you still really do want to construct code as a string without needing TypeOf_xxx.
Nov 26 2020
This seems fine to me now other than the outstanding discussion about code size.
You still appear to have the old patch that doesn't change addDisp.
Nov 25 2020
Nov 24 2020
Nov 23 2020
Currently P0-P1 is valid and results in the program address space being 1, but this patch changes the semantics of that. How sure are you that nothing will break? I do not like the idea of reusing existing valid syntax to mean something else; if you want to introduce some notion of secondary program address spaces then the syntax should be different.
My intuition is that there should be tests for the various instructions that are only hard-float ABIs, and tests specifically for all the different calling conventions that use a very limited set of instructions.