(dup (load) z_or_x_passthrough) => (replicating load)
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
60,030 ms | x64 debian > libFuzzer.libFuzzer::minimize_crash.test |
Event Timeline
llvm/test/tools/llvm-mca/AArch64/A64FX/A64FX-sve-instructions.s | ||
---|---|---|
2574–2575 | I've pushed https://reviews.llvm.org/rGb4028fbc1a88 which fixes this issue. |
- Rebase after b4028fbc1a88
llvm/test/tools/llvm-mca/AArch64/A64FX/A64FX-sve-instructions.s | ||
---|---|---|
2574–2575 | Thanks, much appreciated. Rebased. |
Some test recommendations but otherwise looks good.
llvm/test/CodeGen/AArch64/sve-ld1r.ll | ||
---|---|---|
843 | To cover all the new variants you also want sign/zext forms for the integer types. | |
873 | To cover all the new variants you also want unpacked forms for the floating-point types. | |
963–973 | I don't think dup.x tests are necessary because we don't have any isel patterns for them. Instead we always lower such intrinsics to splat_vector, which is already tested? That said, the dup.x tests are validating the immediate range so perhaps they can be converted to use dup into undef. That that said, I suppose there's an argument that given the immediate operand type is shared by the other pattern within the multiclass it's already being sufficiently tested. It's up to you depending on how paranoid you want to be. |
- Test zext/sext.
- Test unpacked forms (no negative tests, since dup with unpacked types is not specifically supported currently as there is currently no known way to get a DUP_MERGE_PASSTHRU from C/C++ code).
- Drop immediate tests since those are covered.
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | Excuse me, I'm curious why the above changes affect the SideEffects attribute here. Would you give me some guidance? Thank you. |
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | The way that hasSideEffects for an instruction works is that:
If you are looking to remove the side effects flags, and the instruction doesn't have a pattern, adding hasSideEffects=0 is usually a good way to go. Removing the side effects can help improve scheduling freedom and other code motion in the backend, so is usually good to do so long as the instruction doesn't do anything odd. |
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | Thansk @dmgreen very much.
|
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | Yeah - IIRC, I think the logic in Tablegen might only apply when the pattern produces a single instruction. So the pattern with (load (ptrue 31), .. would not set hasSideEffects=0 in the same way as the new (load $pg, .. pattern. |
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | Does hasSideEffects default to false? I investigated this after D140680 and spotted bit hasSideEffects = ?; with none of the base SVE instructions classes that sit above it setting it. I suspect there was some AArch64 upstream refactoring between our original downstream implementation and when it was upstreamed that we missed and so this property has been dandling ever since (either that or it's just always been wrong). Eitherway, as agreed on D140680 I'll have a patch in the next few days that should resolve it. For this patch you can just accept the new value as has been the case of other patches where this flip has occurred. |
llvm/test/tools/llvm-mca/AArch64/Neoverse/N2-sve-instructions.s | ||
---|---|---|
4475–4490 | Oops. Please ignore the final comment, I hadn't spotted this was not a new patch. |
To cover all the new variants you also want sign/zext forms for the integer types.