Change-Id: I0df845749c7689dfc99150ba7c19c7d0dadbd705
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 17573 Build 17573: arc lint + arc unit
Event Timeline
test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll | ||
---|---|---|
144–213 | Is it intentional that there are no ; CHECK lines? |
Hi Luke, thanks for fixing this, some first comments inlined.
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
5982 | Remove comments | |
7792–7794 | nit: trailing whitespace? | |
7798 | nit: don't think you break up the lines like this. | |
lib/Target/AArch64/AArch64InstrInfo.td | ||
4880 | Better not to introduce new line breaks here and also below. | |
test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll | ||
144 | You should add CHECK lines for all these test cases (see examples above). |
test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll | ||
---|---|---|
1 | Please remove this line. | |
14 | Please don't modify the existing test cases. There is no need to check for the %bb.0 stuff as it doesn't add any value. | |
146 | Please remove this line here and similar lines in the test cases below. | |
147 | You should create a regexp for w8, and use it in the line below. Currently the test is very fragile, because if another register gets allocated this test starts failing. |
test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll | ||
---|---|---|
169 | What is the supported range of constant 'n' for this intrinsic? If it is e.g. [1,16], I think it is best to test the minimum value 1, which is what we do here, but also the maximum value 16. |
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
7793 | builds and tests fine without it - removed |
Thanks, looks good to me now. One more nit inlined, but no need for another review.
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
5982 | No changes were made here, so can you keep the old formatting? |
lib/Target/AArch64/AArch64InstrFormats.td | ||
---|---|---|
5982 | Nit: trailing whitespace | |
7793 | Nit: unnecessary new line. | |
7819 | Do the HDr and DHr patterns need to be guarded by predicates [HasNEON, HasFullFP16]? Can you check if they are all predicates are correctly set here? Also looks like we can simply things here a bit: merge all patterns with the same neon and fullfp16 predicates in one block. | |
7834 | To keep the changes minimal, can you please move the "def h" back up where it was? |
- [AArch64] moved FP16 h pattern, refactored FP16 intrinsics into block
- [AArch64] moved FP16 h pattern, refactored FP16 intrinsics into block
Remove comments