[PATCH] D119696: [AMDGPU] Improve v_cmpx usage on GFX10.3.
Sebastian Neubauer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 18 06:50:47 PST 2022
sebastian-ne added a reviewer: sebastian-ne.
sebastian-ne added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:326-330
+ if (VData && VData->getReg() == Reg)
+ return nullptr;
+
+ if (Addr && Addr->isReg() && Addr->getReg() == Reg)
+ return nullptr;
----------------
I’m not sure what I thought before, but a buffer_store obviously only changes memory and not its register arguments, so it’s completely fine if a buffer_store is between the v_cmp and the s_and_saveexec. Checking modifiesRegister should be enough.
================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:356
+ TII->getNamedOperand(SaveExec, AMDGPU::OpName::src0);
+ if (!SaveExecSrc0->isReg() || SaveExecSrc0->getSubReg())
+ return nullptr;
----------------
There’s quite a lot of these isReg and !getSubReg checks in this patch. What are they guarding? I replaced them by asserts and there was no amdgpu codegen test that failed. (Leaving aside the isReg test for v_cmp arguments, which can also be constants.)
================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:645
+ SaveExecInstr->eraseFromParent();
+ VCmpInstr->eraseFromParent();
+ }
----------------
Not sure if this was already mentioned, but do you make sure that the v_cmp is not used anywhere outside the s_saveexec?
I guess it would still be beneficial to insert a v_cmpx if the v_cmp has more uses, but it shouldn’t be removed in that case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119696/new/
https://reviews.llvm.org/D119696
More information about the llvm-commits
mailing list