[PATCH] D30038: [ADMGPU] SDWA peephole optimization pass.

[Sam Kolton via Phabricator via llvm-commits]

SamWot added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:99
+  cl::desc("Enable SDWA peepholer"),
+  cl::init(false));
+
----------------
rampitec wrote:
> Are there plans to enable it?
I want first to submit this patch so that I would be able to fully test it and ensure that it doesn't break anything.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:305-307
+  // Remove original instruction  because it would conflict with our new
+  // instruction by register definition
+  getParentInst()->eraseFromParent();
----------------
arsenm wrote:
> You could create a new virtual register and replace all uses with. If you are going through the trouble of deleting the leftover instructions yourself there's no need to run dead mi elimination again
I have to delete instructions in that case to ensure validity of machine code. Simply intoducing new virtual registers doesn't solve this problem.
Dead code elimination is important for removing src patterns. I don't want to remove by hand because later I want to introduce more complex patterns consisting of several instructions which removal by hand might become to hard.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:476
+
+  // FIXME: For now we only combine instructions in one basic block
+  for (MachineBasicBlock &MBB: MF) {
----------------
arsenm wrote:
> This doesn't seem like a problem to me. I would expect combinable patterns to be sunk down earlier
Same as for enabling pass by default: I want to submit this patch first, then extend it.


https://reviews.llvm.org/D30038