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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:99
+  cl::desc("Enable SDWA peepholer"),
+  cl::init(false));
+
----------------
Are there plans to enable it?


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:23
+
+#include <unordered_map>
+
----------------
arsenm wrote:
> Should be included last. 
It should come after llvm's includes.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:179
+  assert(FirstBB && SecondBB);
+  const MachineFunction *FirstFN = FirstBB->getParent();
+  const MachineFunction *SecondFN = SecondBB->getParent();
----------------
Looks like you only need FirstBB == SecondBB.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:205
+
+MachineRegisterInfo *SDWAOperand::getMRI() const {
+  if (MachineInstr *MI = getParentInst()) {
----------------
arsenm wrote:
> I think this should be removed. There should be no cases where MRI is unavailable, so the null checks here and on its callers are unnecessary. It would be better to just pass around the one MRI reference
Isn't MRI just always available? It is just getParentInst()->getParent()->getRegInfo(). No checks really needed. Most likely this function is not needed too.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:219
+  assert(ParentMI);
+  if (MachineRegisterInfo *MRI = getMRI()) {
+    for (MachineOperand Def: ParentMI->defs()) {
----------------
Check is not needed.


https://reviews.llvm.org/D30038