jyknight added a comment.
In D71726#2182667 <https://reviews.llvm.org/D71726#2182667>, @tra wrote:
>> If a target would like to treat single and double fp atomics as unsupported,
>> it can override the default behavior in its own TargetInfo.
I really don't think this should be a target option at all. Every target can
support the atomic fadd/fsub IR instruction (via lowering to a cmpxchg loop if
nothing else). If it doesn't work, that's a bug in LLVM. We shouldn't be adding
target hooks in Clang to workaround LLVM bugs, rather, we should fix them.
There is one nit -- atomicrmw doesn't (yet) support specifying alignment.
There's work now to fix that, but until that's submitted, only
naturally-aligned atomicrmw instructions can be created. So, for now,
supporting only a naturally-aligned floating-point add would be a reasonable
temporary measure.
> Do we have sufficient test coverage on all platforms to make sure we're not
> generating something that LLVM can't handle everywhere?
Probably not.
> If not, perhaps we should default to unsupported and only enable it for known
> working targets.
No, I don't think that's a good way to go. We should fix LLVM if it's broken.
================
Comment at: clang/lib/CodeGen/CGAtomic.cpp:937
+ if (Val1.isValid())
+ Val1 = Atomics.convertToAtomicIntPointer(Val1);
+ if (Val2.isValid())
----------------
convertToAtomicIntPointer does more than just cast to an int pointer, are you
sure the rest is not necessary for fp types?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:4837
assert(Form != Load);
- if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
+ if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()) ||
+ (IsAddSub && ValType->isFloatingType()))
----------------
This is confusing, and took me a bit to understand what you're doing. I'd
suggest reordering the clauses, putting the pointer case first, e.g.:
```
if (Form == Arithmetic && ValType->isPointerType())
Ty = Context.getPointerDiffType();
else if (Form == Init || Form == Arithmetic)
Ty = ValType;
else if (Form == Copy || Form == Xchg) .....
else ......
...
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71726/new/
https://reviews.llvm.org/D71726
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
