[PATCH] D80749: AMDGPU/GlobalISel: cmp/select method for extract element

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:230-231
+
+  // FIXME: We need another combiner post RegBankSelect. Then move this combine
+  //        there and actually query RegBank of the Idx.
+  bool IsDivergent = false;
----------------
For the 2 element case, we should do this in the pre-legalizer combiner (I guess the 16-bit case code would be different, since it would be bitcast and shift)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:252
+  unsigned NumElem = VecTy.getNumElements();
+  MachineOperand &Vec = MI.getOperand(1);
+  Register Res;
----------------
Register Vec = ...?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:258
+    Register Elt = MRI.createGenericVirtualRegister(EltTy);
+    B.buildExtract(Elt, Vec, I * EltSize);
+    if (I == 0) {
----------------
It would be better to produce a single G_UNMERGE_VALUES and index out of the result list than to produce a sequence of G_EXTRACTs.

Also FYI I'm planning on making 16-bit vector indexing bitcast promote to 32-bit vectors, so this would only see 32 and 64-bit element vectors


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:264-267
+    Register Cmp = MRI.createGenericVirtualRegister(LLT::scalar(1));
+    Register IC = MRI.createGenericVirtualRegister(LLT::scalar(32));
+    B.buildConstant(IC, I);
+    B.buildICmp(CmpInst::ICMP_EQ, Cmp, Idx, IC);
----------------
You can fold these like 
auto B = buildConstant(LLT::scalar(32), ...)
auto Cmp = B.buildICmp(LLT::scalar(1), CmpInst::...)



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:268
+    B.buildICmp(CmpInst::ICMP_EQ, Cmp, Idx, IC);
+    Register Sel = (I == NumElem -1) ? MI.getOperand(0).getReg()
+                                     : MRI.createGenericVirtualRegister(EltTy);
----------------
Weird spacing, NumElem - 1?


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

https://reviews.llvm.org/D80749