[PATCH] D44046: [AMDGPU] Fixed incorrect break from loop

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:353
+  bool SkipAnding = false;
+  if (MI.getOperand(1).isReg())
+    if (MachineInstr *Def = MRI->getUniqueVRegDef(MI.getOperand(1).getReg()))
----------------
Braces


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:354
+  if (MI.getOperand(1).isReg())
+    if (MachineInstr *Def = MRI->getUniqueVRegDef(MI.getOperand(1).getReg()))
+      if (Def->getParent() == MI.getParent())
----------------
Can you add a testcase where this will fail if there is no defining instruction (e.g. the source value was a function argument)


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:356
+      if (Def->getParent() == MI.getParent())
+        SkipAnding = SIInstrInfo::isVALU(*Def);
+
----------------
This doesn't seem like the right condition to me. This should probably be a predicate that checks the specific operand for being a result that is masked with exec? Theoretically this could be something exotic like a write lane.

This also could be = parentmatch && isFoo() rather than an if


================
Comment at: lib/Target/AMDGPU/SILowerControlFlow.cpp:362-363
+    And = BuildMI(MBB, &MI, DL, TII->get(AMDGPU::S_AND_B64), Dst)
+                         .addReg(AMDGPU::EXEC)
+                         .add(MI.getOperand(1));
+    Or = BuildMI(MBB, &MI, DL, TII->get(AMDGPU::S_OR_B64), Dst)
----------------
Identation of these is weird


Repository:
  rL LLVM

https://reviews.llvm.org/D44046