[SPIR-V] OpPhi not handled as a phi node by the MIR verifier #108844
Labels
Comments
Keenuts
added a commit
to Keenuts/llvm-project
that referenced
this issue
Sep 25, 2024
the isPHI() function relied on the MIR opcode. This was fine as AFAIK no real target has a real PHI instruction. With SPIR-V, this assumption breaks. The MachineVerifier has a special handling for PHI instructions to check liveness, but since this relied on the PHI/G_PHI opcode check, it was raising an error when the OpPhi MIR was checked. Since the SPIR-V opcode is specific to the backend, I don't think checking the actual opcode in the MachineVerifier code is correct, so I added a bit in the instruction description, and applied it to the 3 existing PHI instruction I found (G_PHI, PHI, OpPhi). Another different bit is the index of the first BB/reg pair: %res = PHI [reg, BB] %res = OpPhi %reg [reg, BB] The solution I had is to have a function in the MachineInstr returning the start index, allowing the MachineVerifier to work on both formats. Its slightly better, it works, but it's not THAT great. An alternative could be to add the index in the MCInstrDesc, this way, the index bit definition would be in the TD files, closer to the definition. This patch reduces the amount of failling tests with EXPENSIVE_CHECKS from 120 to 113 (All fixed are in the SPIR-V backend). Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
Keenuts
added a commit
to Keenuts/llvm-project
that referenced
this issue
Sep 30, 2024
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an `isPhiInstr`. Alternative designs =================== 1st alternative: ---------------- Add a bit in MCInstDesc for phi instructions (See llvm#110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative: ---------------- Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using `isPHI` to also pass TII. This is a very invasive change, which impacts almost every backends, and several passes. Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
Keenuts
added a commit
to Keenuts/llvm-project
that referenced
this issue
Oct 29, 2024
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an `isPhiInstr`. Alternative designs =================== 1st alternative: ---------------- Add a bit in MCInstDesc for phi instructions (See llvm#110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative: ---------------- Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using `isPHI` to also pass TII. This is a very invasive change, which impacts almost every backends, and several passes. Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
Keenuts
added a commit
to Keenuts/llvm-project
that referenced
this issue
Nov 4, 2024
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an `isPhiInstr`. Alternative designs =================== 1st alternative: ---------------- Add a bit in MCInstDesc for phi instructions (See llvm#110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative: ---------------- Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using `isPHI` to also pass TII. This is a very invasive change, which impacts almost every backends, and several passes. Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
Keenuts
added a commit
to Keenuts/llvm-project
that referenced
this issue
Nov 4, 2024
MachineVerifier can run post-ISel, and the SPIR-V can emit phi nodes. This means G_PHI/PHI are not the only 2 opcodes representing phi nodes. In addition, the SPIR-V instruction operands are ordered slightly differently (1 additional operand before the pairs). There are multiple ways to solve this, but here I decided to go with the less invasive, but less generic method. However the refactoring mentioned in the rest of this commit can still be done later, as it also relies on TII exposing an `isPhiInstr`. Alternative designs =================== 1st alternative: ---------------- Add a bit in MCInstDesc for phi instructions (See llvm#110019). This was refused as the MCInstDesc bits are "pricy", and a that only impacts 3 instructions is not a wise expense. 2nd alternative: ---------------- Refactorize MachineInstr::isPHI() to use TargetInstrInfo. This was almost possible, as MachineInstr often has a parent MBB, and thus a parent MF which links to the TargetInstrInfo. In MachineInstr: this->getMF().getTargetInstrInfo().isPhiInstr(*this) This however breaks as soon as the MachineInstr is removed from it's parent block. Solving this requires passing TII as a parameter to isPHI. Passing TII requires each code using `isPHI` to also pass TII. This is a very invasive change, which impacts almost every backends, and several passes. Fixes llvm#108844 Signed-off-by: Nathan Gauër <[email protected]>
|
MIR verifier tests now pass with expensive checks enabled thanks to broxigarchen@affad1b |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When expensive checks are enabled, MIR is checked after each pass.
SPIR-V has an OpPhi instruction, similar to LLVM's phi instruction.
But because the MIR verifier doesn't know about it, it expects all operands to be defined by a dominating block (unlike the phi instruction).
Might want to add a new bit in the td files (akin to isBarrier bit) so the verifier can know "this instruction is allowed to have partially undefined operands)?
The text was updated successfully, but these errors were encountered: