Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some comments.
clang/include/clang/AST/ASTContext.h | ||
---|---|---|
1117 ↗ | (On Diff #409186) | Is this necessary? |
clang/lib/AST/ASTContext.cpp | ||
1330 | Is this necessary? | |
6810–6812 | This can be left unmodified. | |
clang/test/CodeGen/aarch64-complex-half-math.c | ||
2 | Can these test outputs be autogenerated with update_cc_test_checks.py? | |
3 | I think this test needs a // REQUIRES: aarch64-registered-target to protect builds which don't have a LLVM_TARGETS_TO_BUILD which implies AArch64 is available. | |
6 | My read is that this appears to be testing both -ffast-math and _Float16. Do we want to be testing both these things at once like this, can anyone comment? My preference would be to have these test only the operators, and not the flags, and then maybe have a single test which does #pragma clang fp contract(fast) and verifies that the flags are present. |
clang/test/CodeGen/aarch64-complex-half-math.c | ||
---|---|---|
2 | Do the tests need to depend on LLVM optimizations? In general this is undesirable as it makes them more fragile if things change on the LLVM side. |
clang/test/CodeGen/aarch64-complex-half-math.c | ||
---|---|---|
3 | There's nothing AArch64-specific about the change, so I'd expect some more general codegen tests. Actually, I took a look to see what calls getFloatingTypeOfSizeWithinDomain() and I can't find any callers of that. Do you have a test case where you were hitting that particular unreachable? |
clang/test/CodeGen/aarch64-complex-half-math.c | ||
---|---|---|
3 | Thanks for your observation @aaron.ballman. It appears this has been working since D105331, which enabled it for X86. Unless I'm missing something, the function in question appears to have been unused since 2010 (the last use was removed in d005ac937e4c08e0c607ddc42708f8bf2064c243 by @rjmccall), I presume getFloatingTypeOfSizeWithinDomain should be removed then. |
clang/test/CodeGen/aarch64-complex-half-math.c | ||
---|---|---|
3 |
Agreed; I've gone ahead and removed it in 841355c1e4e35fc02b5b171419979f5f9af0ebc8. I think this patch can be abandoned, unless you think the extra test coverage is worth adding as an NFC commit? |
Abandoning this patch as it is redundant due to changes in D105331. This patch also drew attention to unused code that has since been removed in 841355c1e4e35fc02b5b171419979f5f9af0ebc8. Many thanks to @aaron.ballman
Is this necessary?