[PATCH] D119696: [AMDGPU] Improve v_cmpx usage on GFX10.3.

[Thomas Symalla via Phabricator via llvm-commits]

tsymalla added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:356
+      TII->getNamedOperand(SaveExec, AMDGPU::OpName::src0);
+  if (!SaveExecSrc0->isReg() || SaveExecSrc0->getSubReg())
+    return nullptr;
----------------
sebastian-ne wrote:
> tsymalla wrote:
> > sebastian-ne wrote:
> > > tsymalla wrote:
> > > > sebastian-ne wrote:
> > > > > 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.)
> > > > Regarding the v_cmp arguments, these are constants as you mentioned so these checks make sense.
> > > > However, I think we can safely assume that the check for s_and_saveexec will always pass as the arguments are registers, so I'm going to remove these.
> > > Did you remove any of these checks? I still see a lot of isReg and getSubReg checks
> > Yes, I removed the check around the s_and_saveexec operand. For the v_cmp checks, either src0 or src1 could be a register and shouldn't be a subregister, so I think these checks still make sense.
> Ah, I missed that. I’m still wondering what the getSubReg checks do?
> There’s no change on the llvm tests with or without them. I guess a test that hits these code paths would be nice?
As far as I understood, after register allocation, no subregister indices should be used as we are dealing with physical registers. In Post-SSA there can still be virtual registers. These subregs can be used to get lane bitmasks which represent the parts that are covered by the subregister index if the register is still virtual. So, this should be a check for completeness, but it's probably not necessary here. I have seen some assertions in the codegen where they assume the register does not consist of subregs anymore post-ra (where SIOptimizeExecMaskingPass fits in), So probably it's nice to have, but would not affect tests either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119696/new/

https://reviews.llvm.org/D119696