[PATCH] D53496: AMDGPU: Rewrite SILowerI1Copies to always stay on SALU

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Thanks for taking a look.



================
Comment at: lib/Target/AMDGPU/SIFixSGPRCopies.cpp:186
                              const SIRegisterInfo &TRI) {
-  return TRI.isSGPRClass(DstRC) && TRI.hasVGPRs(SrcRC);
+  return SrcRC != &AMDGPU::VReg_1RegClass && TRI.isSGPRClass(DstRC) &&
+         TRI.hasVGPRs(SrcRC);
----------------
arsenm wrote:
> Could we avoid still having VReg_1 by checking for i1 source instructions?
I don't know what you mean. At this point in the compilation we can't link things back to IR anymore, so VReg_1 is actually a convenient way of indicating that a value was originally an i1.


================
Comment at: lib/Target/AMDGPU/SILowerI1Copies.cpp:88-90
+    if (Reg == AMDGPU::VCC || Reg == AMDGPU::EXEC)
+      return true;
+
----------------
arsenm wrote:
> Should this worry about sub registers? Can't this just use TRI.getRegClass instead of worrying about physical registers itself
Simplified to using TRI instead of listing register classes explicitly.

I don't think subregisters come into it at this point. At least the cases I've tried all ended up scalarized and I was unable to get subregisters to appear. Anyway, I'm adding some assertions about subregisters just in case.


================
Comment at: lib/Target/AMDGPU/SILowerI1Copies.cpp:164-175
+      // If this block has a divergent terminator and the def block is its
+      // post-dominator, the wave may first visit the other successors.
+      bool Divergent = false;
+      for (MachineInstr &MI : MBB->terminators()) {
+        if (MI.getOpcode() == AMDGPU::SI_NON_UNIFORM_BRCOND_PSEUDO ||
+            MI.getOpcode() == AMDGPU::SI_IF ||
+            MI.getOpcode() == AMDGPU::SI_ELSE ||
----------------
arsenm wrote:
> Separate function?
I don't think that helps.


================
Comment at: lib/Target/AMDGPU/SILowerI1Copies.cpp:513-514
+    for (MachineInstr &MI : MBB.phis()) {
+      if (!MI.isPHI())
+        continue;
+
----------------
arsenm wrote:
> Why is this necessary with phis()
You're right, it's not.


================
Comment at: lib/Target/AMDGPU/SILowerI1Copies.cpp:695
+    MI = MRI->getUniqueVRegDef(Reg);
+    if (MI->getOpcode() != AMDGPU::COPY)
+      break;
----------------
rampitec wrote:
> MI can be null technically.
How? This pass runs on Machine SSA form.


================
Comment at: lib/Target/AMDGPU/SILowerI1Copies.cpp:771
+  if (PrevConstant && !PrevVal) {
+    BuildMI(MBB, I, DL, TII->get(AMDGPU::COPY), DstReg).addReg(CurMaskedReg);
+  } else if (CurConstant && !CurVal) {
----------------
arsenm wrote:
> .addRegs on new line
clang-format disagreed :/

But yeah, I'm changing it back.


Repository:
  rL LLVM

https://reviews.llvm.org/D53496