[compiler][arm64] Remove OneLaneSwizzle [v8/v8 : main]
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 3 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
3 comments
InstructionOperand temp = g.TempSimd128Register();
Emit(kArm64S128MoveReg, temp, g.UseRegister(input0));So I could be wrong because I get confused by UseRegister vs UseUniqueRegister every few months, but I don't think that you are guaranteed that `temp` and `g.UseRegister(input0)` don't alias. And if that happens, the `DCHECK_NE(dst, src);` you added in the code generator will trigger. I think that you need `UseUniqueRegister(input0)` instead of `UseRegister(input0)` (also in the next Emit maybe).
Emit(kArm64S128MoveLane | LaneSizeField::encode(32),
g.DefineSameAsFirst(node), temp, g.UseRegister(input0),
g.UseImmediate(from), g.UseImmediate(to));Is this covered by some tests? There is a `DCHECK_EQ(dst, i.InputSimd128Register(0).Format(f));` in the codegen when processing kArm64S128MoveLane and I think that this would require `temp` and `g.DefineSameAsFirst(node)` to alias, and I don't see how you're guaranteeing this. Please add a test.
Related details
- Nico Hartmann
- Sam Parker-Haynes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 2 comments![]( "Open in Gerrit")
InstructionOperand temp = g.TempSimd128Register();
Emit(kArm64S128MoveReg, temp, g.UseRegister(input0));So I could be wrong because I get confused by UseRegister vs UseUniqueRegister every few months, but I don't think that you are guaranteed that `temp` and `g.UseRegister(input0)` don't alias. And if that happens, the `DCHECK_NE(dst, src);` you added in the code generator will trigger. I think that you need `UseUniqueRegister(input0)` instead of `UseRegister(input0)` (also in the next Emit maybe).
Ah, interesting. I do feel like the instruction selector is much harder to work with than it should be! I will try to remember this in the future, thanks.
Emit(kArm64S128MoveLane | LaneSizeField::encode(32),
g.DefineSameAsFirst(node), temp, g.UseRegister(input0),
g.UseImmediate(from), g.UseImmediate(to));Is this covered by some tests? There is a `DCHECK_EQ(dst, i.InputSimd128Register(0).Format(f));` in the codegen when processing kArm64S128MoveLane and I think that this would require `temp` and `g.DefineSameAsFirst(node)` to alias, and I don't see how you're guaranteeing this. Please add a test.
Sorry, I guess I misunderstand how `DefineSameAsFirst` works then, I thought the whole purpose of it would be to use `temp`? I hope it's guaranteed..? Anyway, a test was indeed missing.
Related details
- Darius Mercadier
- Nico Hartmann
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier voted and added 3 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
3 comments
InstructionOperand temp = g.TempSimd128Register();
Emit(kArm64S128MoveReg, temp, g.UseRegister(input0));Sam Parker-HaynesSo I could be wrong because I get confused by UseRegister vs UseUniqueRegister every few months, but I don't think that you are guaranteed that `temp` and `g.UseRegister(input0)` don't alias. And if that happens, the `DCHECK_NE(dst, src);` you added in the code generator will trigger. I think that you need `UseUniqueRegister(input0)` instead of `UseRegister(input0)` (also in the next Emit maybe).
Ah, interesting. I do feel like the instruction selector is much harder to work with than it should be! I will try to remember this in the future, thanks.
I do feel like the instruction selector is much harder to work with than it should be
Welcome to the club :p
Emit(kArm64S128MoveLane | LaneSizeField::encode(32),
g.DefineSameAsFirst(node), temp, g.UseRegister(input0),
g.UseImmediate(from), g.UseImmediate(to));Sam Parker-HaynesIs this covered by some tests? There is a `DCHECK_EQ(dst, i.InputSimd128Register(0).Format(f));` in the codegen when processing kArm64S128MoveLane and I think that this would require `temp` and `g.DefineSameAsFirst(node)` to alias, and I don't see how you're guaranteeing this. Please add a test.
Sorry, I guess I misunderstand how `DefineSameAsFirst` works then, I thought the whole purpose of it would be to use `temp`? I hope it's guaranteed..? Anyway, a test was indeed missing.
Ah right, sorry I'm the one who got confused; I thought that `DefineSameAsFirst` referred to the Turboshaft input of `node` but it does indeed refer to the input of the Instruction, so you're good, nevermind :)
Related details
- Nico Hartmann
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Hi Matthias, could you review my simd shuffle fix please?
Related details
- Matthias Liedtke
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 2 comments![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
2 comments
LGTM
Hi Matthias, could you review my simd shuffle fix please?
Sorry, it's my first day after an extended leave, still trying to catch up with my inbox. 😊
Related details
- Nico Hartmann
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Matthias LiedtkeHi Matthias, could you review my simd shuffle fix please?
Sorry, it's my first day after an extended leave, still trying to catch up with my inbox. 😊
No problem, I only added you ten minutes before :) Welcome back!
Related details
- Nico Hartmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[compiler][arm64] Remove OneLaneSwizzle
Introduce S128MoveReg and use that, along with MoveLane, to implement
the same functionality in isel.
- M src/compiler/backend/arm64/code-generator-arm64.cc
- M src/compiler/backend/arm64/instruction-codes-arm64.h
- M src/compiler/backend/arm64/instruction-scheduler-arm64.cc
- M src/compiler/backend/arm64/instruction-selector-arm64.cc
- M src/wasm/simd-shuffle.cc
- M test/unittests/compiler/arm64/turboshaft-instruction-selector-arm64-unittest.cc
Code-Review: +1 by Darius Mercadier, +1 by Matthias Liedtke
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |