- User Since
- Oct 18 2012, 4:53 AM (309 w, 2 d)
Fri, Sep 21
I've had a go at producing an example too with no luck. The Constant involved on the breaking config is ridiculously complex (even after reduction): i32 or (i32 shl (i32 or (i32 shl (i32 or (i32 shl (i32 and (i32 ptrtoint ([19 x i8]* @0 to i32), i32 63), i32 8), i32 and (i32 lshr (i32 ptrtoint ([19 x i8]* @0 to i32), i32 6), i32 63)), i32 8), i32 and (i32 lshr (i32 lshr (i32 ptrtoint ([19 x i8]* @0 to i32), i32 6), i32 6), i32 63)), i32 8), i32 lshr (i32 lshr (i32 lshr (i32 ptrtoint ([19 x i8]* @0 to i32), i32 6), i32 6), i32 6))
Thread necromancy! I realised that I got distracted and never finished this discussion.
Thu, Sep 20
Thu, Sep 13
Thanks. Committed as r342127.
Wed, Sep 12
I think I've implemented the suggested changes, except for the function alignment one.
Mon, Sep 10
We should also be aligning functions?
Fri, Sep 7
Thanks. Committed as r341642.
Thu, Sep 6
Added the addressing-mode to ARMFrameLowering. The only other place that uses them appears to be optional (for an efficiency gain), so I skipped that.
Wed, Sep 5
Bother, yes I do. That's going to be fun to write a test for so it'll probably have to wait until tomorrow.
They'd probably work. We often just use useful triples, maybe thumbv8-linux-gnueabihf, and perhaps thumbv7m-linux-gnueabi as a soft-float target that ought to use libcalls for everything (I'd probably just check there are the appropriate number of calls there rather than tracking all the marshalling nonsense that goes on).
Looks good to me.
Could you possibly duplicate the tests for ARM? It looks like the patch does the right thing to me, but it'd be good to have it confirmed and tested.
Aug 23 2018
Aug 20 2018
EmitAArch64BuiltinExpr() also emits args into Ops before the big switch (with some more subtlety around the last arg that I don't understand), but then almost every switch case does EmitScalarExpr(E->getArg(n)).
Aug 6 2018
Thanks for the comments. I've committed basically the original version of the D40404 (modulo DEBUG -> LLVM_DEBUG renaming) as r339009.
Aug 3 2018
Aug 1 2018
I think it's reasonable too.
I suppose that this is caused by the pair-wise split relocations à la IMAGE_REL_ARM_MOV32Tfor PE/COFF
Looks fine to me.
Jul 31 2018
IMO, linker optimization hints are more an AArch64 thing than a Darwin thing. They were invented entirely to deal with AArch64's ADRP/whatever sequences so I'd prefer the implementation to stay in lib/Target/AArch64 where possible.
Jul 18 2018
I've committed this as r337389. Thanks for submitting the patch.
Jul 6 2018
Thanks Duncan. Committed as r336419.
Jul 5 2018
Address review comments and rework iteration to always find next location before modifying the current one.
Thanks for looking at the patch.
Jul 4 2018
Jun 28 2018
This looks reasonable to me.
Jun 25 2018
I'd rather not put target names in API functions. The meaning of that field is pretty target independent ("alignment of type, before alignment adjustments")
Jun 22 2018
I'm fine with the ABI changes, but I'm not very convinced by the "NaturalAlignment" name.
Jun 20 2018
Sorry, I meant to do this a few days ago but forgot to actually commit and just rediscovered it today. It's pushed as r335118 now.
Jun 19 2018
Added t2ADDi12 and tests for the immediate cases.
Jun 18 2018
Fixing cmpxchg implementation of load to return correct value.
Sorry I take so long replying to this each time. I really need to plan my time better.
Switched to T1Pat on Thumb-1 side and added t2ADDrr pattern.
Jun 15 2018
Jun 14 2018
It seems a bit of a shame to hide that masked expansion away in lib/Target/RISCV when MIPS (at least) needs similar logic. I suppose someone who wants to refactor MIPS can move it out again though.
I obviously haven't been following RISC-V enough to give final reviews, but this mostly looked sensible to me. Just one question:
I think this looks reasonable to me.
May 24 2018
I think this is fine.
Looks reasonable to me.
May 11 2018
The Linux libatomic __atomic_is_lock_free returns false for unaligned pointers, even on x86. clang must generate code which is compatible with that, so it *cannot* inline misaligned atomic operations.
Apr 26 2018
I think I prefer Geoff's solution too. Better to make it work than just turn off parts of the optimizer, and it's not even particularly more difficult to read.
Apr 23 2018
Ah, I see you approved it before, presumably with the suggested changes. I've committed as r330566.
Moved diagnostic emission next to where the decision is made.
Yep, I think that ought to work. Sorry about the delay.
Apr 19 2018
Apr 13 2018
Sorry to update the patch after you've reviewed it, but I'm afraid as a result of https://llvm.org/PR34347 it's turned out that misaligned x86 atomics actually need the lock-free implementation, which makes this even messier than I thought.
Apr 12 2018
Thanks for updating the patch. LGTM!
Apr 10 2018
Looks fine to me.
Apr 9 2018
Updated so that 16-byte atomics work when the instruction is always available. Good idea Eli.
Fair enough, thanks for looking into it. LGTM!
Apr 7 2018
I managed to track down the issue, and I think the patch needs to respect the "uwtable" attribute too (otherwise the tables produced are useless). So I've recommitted with that change applied; hopefully it'll stick this time.
Apr 6 2018
Apr 5 2018
Bug won't clang lower these access to the exactly that IR that LLVM declares as UB?
Also, on the LLVM side there are even fewer restrictions and load atomic i32, i32* %ptr monotonic, align 1 is perfectly valid IR that gets lowered to these calls.
Sorry about the delay, I've committed this for you as r329287.
Atomics accessed via C11 _Atomic and C++11 std::atomic will be suitably aligned, but there's a reasonable amount of legacy code that uses the GCC builtins on non-atomic types (of unknown alignment) and this is what Clang uses to implement those accesses when they come up. Also, on the LLVM side there are even fewer restrictions and load atomic i32, i32* %ptr monotonic, align 1 is perfectly valid IR that gets lowered to these calls.
Mar 27 2018
The warning you're seeing is because without an operand modifier Clang always chooses an x-register in its textual assembly expansion. This 64-bit default is compared to the underlying C type to determine whether a warning should be emitted, and I think that's reasonable. The C type is all that's available for most uses (without a register keyword) and it seems consistent to use it even when register has been specified on the variable.
Mar 24 2018
A nice little change with big benefits! One little nit (no need to reupload here after fixing, I think):
Mar 23 2018
Looks like it's clearly an improvement, and very low risk so it'd just as well go into 6.0 if there's time.
Looks reasonable to me.
Mar 19 2018
Thanks. This looks fine now too.
Thanks. I think this looks reasonable now.
Mar 17 2018
Looks fine to me.
Mar 16 2018
Yep, I thought the bitconverts were a good idea, but should also have the variants that trigger for IsBE.
Why is this better?
Ah good, thanks for the explanation. Sounds like you were already doing exactly what I was thinking of and I started paying attention half-way through.
Mar 15 2018
It looks like there's nothing testing the bitconvert patterns here.
These look fine to me.
These look right to me.
Dec 10 2017
Thanks Richard, and all other reviewers. I committed this as r320250, with a couple of sanitizer test fixes as r320251 and r320284 (thanks Ahmed!).
Dec 8 2017
Updating with tentative fixes to review comments.
Dec 7 2017
Thanks Richard. I'll file the bugs tomorrow for the issues you suggest. Do you see either of them blocking the change to C++14 as a default? On a scale of "no", "no but I want a commitment to fix them" and "yes" sort of thing.