[PATCH] D147168: [AMDGPU] Introduce SIInstrWorklist to process instructions in moveToVALU

[krishna chaitanya sankisa via Phabricator via llvm-commits]

skc7 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:872
   if (!SrcReg.isVirtual() || TRI->isAGPR(*MRI, SrcReg)) {
-    TII->moveToVALU(MI, MDT);
+    SIInstrWorklist worklist;
+    worklist.insert(&MI);
----------------
arsenm wrote:
> Should this avoid being reconstructed for each handled case?
SIInstrWorklist  is needed for every moveToVALU call. In lowerVGPR2SGPRCopies method, all the copies are being pushed to worklist and one call to moveToVALU would be made. But in other methods, a new worklist object needs to be created and passed to moveToVALU.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6154-6155
+void SIInstrWorklist::insert(MachineInstr *MI) {
+  auto itr1 = std::find(InstrList.begin(), InstrList.end(), MI);
+  if (itr1 == InstrList.end())
+    InstrList.push_back(MI);
----------------
arsenm wrote:
> Why did you revert from using a SetVector for this case?
The previous patch was a WIP, I was trying to experiment other things. Now, moved worklists to setVectors


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6193-6195
+void SIInstrInfo::moveToVALUimpl(SIInstrWorklist &Worklist,
+                                 MachineDominatorTree *MDT,
+                                 MachineInstr &Inst) const {
----------------
arsenm wrote:
> Capitalize Impl. You can also do the refactor to use the implicit function with a different typedef for the wordlist in a precommit
Changed to moveToVALUImpl. This method is tied to this current implementation, where it uses newly created SIInstrWorklist . Not sure, how can it be made into precommit. 


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6341
+    Inst.removeOperand(1);
+  } break;
+
----------------
arsenm wrote:
> whitespace error
Ran clang-format before submitting the patch. Should I move the break below the curly braces?


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6488-6489
+    const TargetRegisterClass *NewDstRC = getDestEquivalentVGPRClass(Inst);
+    if (!NewDstRC)
+      return;
+
----------------
arsenm wrote:
> Don't understand why this null check was introduced 
It was part of WIP patch. Was trying to fix few tests. Removed it.


================
Comment at: llvm/test/CodeGen/AMDGPU/control-flow-fastregalloc.ll:13-23
+; VMEM-LABEL: divergent_if_endif:
+; VMEM:         .amd_kernel_code_t
+; VMEM-NEXT:     amd_code_version_major = 1
+; VMEM-NEXT:     amd_code_version_minor = 2
+; VMEM-NEXT:     amd_machine_kind = 1
+; VMEM-NEXT:     amd_machine_version_major = 7
+; VMEM-NEXT:     amd_machine_version_minor = 0
----------------
arsenm wrote:
> We don't want checks for all this old style metadata. If you're going to generate the checks, use a triple with a newer default code object. Also convert tests to generated checks in a pre-commit
Now updated the check lines which were failing. Reverted from generating automatic check-lines with script.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147168