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 |