[PATCH] D67662: [AMDGPU] SIFoldOperands should not fold register acrocc the EXEC definition

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:422
+
+static bool isFoldingRegisterAcrossExecDef(const MachineInstr &MI,
+                                          const MachineOperand &UseMO) {
----------------
We have execMayBeModifiedBeforeUse(), although it does not do cross block scan. I.e. you need to extend execMayBeModifiedBeforeUse() instead of creating another function with the same intent.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:423
+static bool isFoldingRegisterAcrossExecDef(const MachineInstr &MI,
+                                          const MachineOperand &UseMO) {
+  const MachineInstr *Def = UseMO.getParent();
----------------
Formatting is off.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:428
+  bool CrossDefExec = false;
+  if (!const_cast<MachineBasicBlock *>(DefBB)->canFallThrough() &&
+      (DefBB != MBB)) {
----------------
Even if it can fall through exec can be modified.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:429
+  if (!const_cast<MachineBasicBlock *>(DefBB)->canFallThrough() &&
+      (DefBB != MBB)) {
+    MachineBasicBlock::const_iterator IT =
----------------
What if there are several blocks and branches in between of def and use?


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:433
+    const MachineInstr *Term = &*IT;
+    CrossDefExec = Def->hasRegisterImplicitUseOperand(AMDGPU::EXEC) &&
+                   Term->definesRegister(AMDGPU::EXEC);
----------------
Just Def->readsRegister(AMDGPU::EXEC, TRI).


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:434
+    CrossDefExec = Def->hasRegisterImplicitUseOperand(AMDGPU::EXEC) &&
+                   Term->definesRegister(AMDGPU::EXEC);
+  }
----------------
modifiesRegister(ANDGPU::EXEC, TRI). Otherwise it will not work in wave32.


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

https://reviews.llvm.org/D67662