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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:174
+
+bool isSameBB(const MachineInstr *FirstMI, const MachineInstr *SecondMI) {
+  assert(FirstMI && SecondMI);
----------------
SamWot wrote:
> arsenm wrote:
> > This function shouldn't be necessary. I think all you want to do is check the blocks have the same parent? You shouldn't be seeing situations where you're seeing instructions from multiple functions
> Yes, this is overkill but if I check toher thing here then why not to check that functions are also same, just for confidence?
You do not check the module is the same? This is function pass, you do not get blocks from different functions.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:179
+  assert(FirstBB && SecondBB);
+  const MachineFunction *FirstFN = FirstBB->getParent();
+  const MachineFunction *SecondFN = SecondBB->getParent();
----------------
SamWot wrote:
> rampitec wrote:
> > Looks like you only need FirstBB == SecondBB.
> This should work but I don't want to campare pointers. If internal structure of MachineFunction changes later then this would not work. Also basic block number is exactly what we need to check if this is same basic block - then why not to use it?
> What I should really fix is that  `FirstMI->getParent()->getNumber();` should be `FirstBB->getNumber()`.
All llvm works this way. BTW, numbers can change as well. You are taking parent basic block pointers from both instructions right here. Do you expect blocks to change in between of these two getParent() calls?


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:466
+  if (!ST.hasSDWA() ||
+      !AMDGPU::isVI(ST)) { // FIXME: We don't support SDWA anywhere other than VI
+    return false;
----------------
SamWot wrote:
> rampitec wrote:
> > arsenm wrote:
> > > This check is unnecessary?
> > ```
> > >= VOLCANIC_ISLANDS?
> > ```
> We don't yet support SDWA on gfx9. 
It then needs TODO comment here.


https://reviews.llvm.org/D30038