Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
21413 | I think ComplxNum requires NEON, so checking for the first should be enough. | |
21460 | There is quite a lot of debug message left here. I assume it will be removed in the final version? | |
llvm/test/CodeGen/AArch64/ComplexArithmetic/complex-arithmetic-f16-add.ll | ||
1 | These don't need to be in a "ComplexArithmetic" directory - they are already easy to identify from the file prefix. | |
6 | Can you try and clean up these newlines too. There are gaps in places, but some of the functions are a little jammed together. |
Can you add tests cases for some of the awkward cases we found in D114174, like the <12 x > vectors (if they are not present already).
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
21412 | Can you add some extra newlines between methods | |
21413 | This I don't think needs the NEON check. | |
21428 | Formatting, here and below. | |
21443 | Add a message to the assert |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
21450 | We do, but it's also backed up by the if ((VTyWidth < 128 && VTyWidth != 64) || !llvm::isPowerOf2_32(VTyWidth)) above. Though changing the assert to reflect this restriction might be best, rather than assuming that it's met. | |
21464 | This check is needed, however the previous assignment is not. I've moved it down to where it is needed |
Cool, thanks.
Looks like a nice change to me. Will leave further reviews/approval up to Dave.
I was hoping we could get some correctness results for the MVE side overnight, but out internal testing seems pretty broken for other reasons. I don't see anything that is complex number related, so hopefully this is OK.
So long as you have done some testing, LGTM.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
21444–21445 | Formatting, and it probably wants brackets around the || |
Can you add some extra newlines between methods