Details
- Reviewers
craig.topper zvi igorb RKSimon m_zuckerman - Commits
- rG5bf6620b1523: [X86][AVX512] Adding a pattern for broadcastm intrinsic.
rG70280f9a0dbb: [X86][AVX512] Adding a pattern for broadcastm intrinsic.
rG98c7f91e5457: pre-commit adding test for broadcastm pattern
rL316921: [X86][AVX512] Adding a pattern for broadcastm intrinsic.
rL316890: [X86][AVX512] Adding a pattern for broadcastm intrinsic.
rL314626: pre-commit adding test for broadcastm pattern
Diff Detail
- Repository
- rL LLVM
Event Timeline
Please remember to create your diff with context: http://llvm.org/docs/Phabricator.html
test/CodeGen/X86/broadcastm-lowering.ll | ||
---|---|---|
3 ↗ | (On Diff #116791) | Please don't use -mcpu, use -mattr instead. Something like: ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mcpu=+avx512f | FileCheck %s --check-prefix=ALL --check-prefix=AVX512F ; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+avx512vl,+avx512dq | FileCheck %s --check-prefix=ALL --check-prefix=AVX512DQVL Also, should i686 triples be tested as well? Not sure if any non-legal i64 issues will arise |
The title should have an X86 label, and can you please add a cover message in the summary?
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6695 ↗ | (On Diff #116791) | Would isSplatZeroExtended be a better name? |
6749 ↗ | (On Diff #116791) | Please add a comment explaining the change here and in the Summary. Maybe something like: |
6749 ↗ | (On Diff #116791) | Checking |
test/CodeGen/X86/broadcastm-lowering.ll | ||
---|---|---|
2 ↗ | (On Diff #116791) | instead of -mcpu=skx , you should use -mattr which is more robust to scheduling changes: and similarly for knl: |
test/CodeGen/X86/broadcastm-lowering.ll | ||
---|---|---|
3 ↗ | (On Diff #116791) | Simon has already mentioned this |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6605 ↗ | (On Diff #116926) | vedtor->vector Also the first word of each sentence in the comment should be capitalized. |
6628 ↗ | (On Diff #116926) | I think this line should be indented 2 additional spaces? |
6635 ↗ | (On Diff #116926) | Am I missing something or is this variable and several above it dead? |
6667 ↗ | (On Diff #116926) | Use is512BitVector here. |
6674 ↗ | (On Diff #116926) | Does this overflow 80 columns? |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6635 ↗ | (On Diff #116926) | those variables are sending as reference to this function. |
test/CodeGen/X86/broadcastm-lowering.ll | ||
---|---|---|
3 ↗ | (On Diff #116791) | Please can you tidy the prefixes: AVX512CD, AVX512VLCDBW and X86-AVX512VLCDBW would be more closely match convention. |
Thanks please can you now commit broadcastm-lowering.ll with trunk's current codegen so that this patch shows the codegen diff?
A few minor comments
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6608 ↗ | (On Diff #117286) | Describe the function NumElt and EltType, also I'm not convinced that 'isSplatZeroExtended' is suitably descriptive of what the function is actually doing. |
6611 ↗ | (On Diff #117286) | unsigned NumElts = Op->getNumOperands(); unsigned Delta = NumElts; |
6613 ↗ | (On Diff #117286) | Comment describing the search you're doing in the loop for (unsigned i = 1; i < NumElts; i++) { |
6624 ↗ | (On Diff #117286) | Comment the loop for (unsigned i = Delta; i < NumElts; i++) { |
6633 ↗ | (On Diff #117286) | Op->getSimpleValueType(0).getScalaraSizeInBits() |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6608 ↗ | (On Diff #117286) | i could not find a better name. do you have one? |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6605 ↗ | (On Diff #117302) | Please, add example of the pattern that you are looking for |
6639 ↗ | (On Diff #117302) | Use NumElts /Delta |
6673 ↗ | (On Diff #117302) | Why do you set the NumElt to one? |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6674 ↗ | (On Diff #117302) | if(NumElts!=1&&SDValue ZeroExtended = isSplatZeroExtended(BVOp, NumElts, EltType)) |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
6674 ↗ | (On Diff #117302) | sorry without NumElts!=1 |