Page MenuHomePhabricator

GlobalISel/InstructionSelector: Implement GIR_CopyFConstantAsFPImm
ClosedPublic

Authored by tstellar on Apr 23 2018, 2:45 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Apr 23 2018, 2:45 PM
dsanders accepted this revision.May 1 2018, 10:51 AM

LGTM. Preferably with a test case in this patch but adding the test cases along with the AMDGPU patches you're working on will also be ok

This revision is now accepted and ready to land.May 1 2018, 10:51 AM

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

Could you send me the comment that's shortly before the GIR_CopyFConstantAsFPImm in AMDGPUGenGlobalIsel.inc? It should look similar to a tablegen pattern and we can probably derive a test case from that. I'm not sure which rule has been imported but I'm hoping it will be something like (set $rd:GPR, $imm:fpimm) in which case the test would be similar to test/CodeGen/AArch64/GlobalISel/select-constant.mir.

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

Could you send me the comment that's shortly before the GIR_CopyFConstantAsFPImm in AMDGPUGenGlobalIsel.inc? It should look similar to a tablegen pattern and we can probably derive a test case from that. I'm not sure which rule has been imported but I'm hoping it will be something like (set $rd:GPR, $imm:fpimm) in which case the test would be similar to test/CodeGen/AArch64/GlobalISel/select-constant.mir.

Here is the comment. Note that the reason I don't have a test case is because this pattern is only for the R600 sub-target and I'm not implementing global-isel for that sub-target:

// MIs[0] Operand 1
// No operand predicates
// (fpimm:{ *:[f32] }):$val  =>  (MOV_IMM_F32:{ *:[f32] } (fpimm:{ *:[f32] }):$val)
GIR_BuildMI, /*InsnID*/0, /*Opcode*/AMDGPU::MOV_IMM_F32,
GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
GIR_CopyFConstantAsFPImm, /*NewInsnID*/0, /*OldInsnID*/0, // va

I actually don't have a test case of this. I really just need the enum defined so AMDGPUGenGlobalIsel.inc will build. I can drop the implementation and just add the enum value if you prefer.

Could you send me the comment that's shortly before the GIR_CopyFConstantAsFPImm in AMDGPUGenGlobalIsel.inc? It should look similar to a tablegen pattern and we can probably derive a test case from that. I'm not sure which rule has been imported but I'm hoping it will be something like (set $rd:GPR, $imm:fpimm) in which case the test would be similar to test/CodeGen/AArch64/GlobalISel/select-constant.mir.

Here is the comment. Note that the reason I don't have a test case is because this pattern is only for the R600 sub-target and I'm not implementing global-isel for that sub-target:

// MIs[0] Operand 1
// No operand predicates
// (fpimm:{ *:[f32] }):$val  =>  (MOV_IMM_F32:{ *:[f32] } (fpimm:{ *:[f32] }):$val)
GIR_BuildMI, /*InsnID*/0, /*Opcode*/AMDGPU::MOV_IMM_F32,
GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
GIR_CopyFConstantAsFPImm, /*NewInsnID*/0, /*OldInsnID*/0, // va

Thanks, something similar select-constant.mir would do the trick for that one but seeing as it's for a target that you aren't implementing it makes sense to go ahead without it. I think we might as well keep the implementation seeing as we have it but could you add a TODO indicating that it needs a test case as soon as one is possible?

This revision was automatically updated to reflect the committed changes.