[PATCH] D138240: AMDGPU: Introduce function pass version of LowerIntrinsics

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez added a comment.

Looks good to me, I just added few cosmetical remarks.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerIntrinsics.cpp:135
+    IntrinsicInst *Inst = cast<IntrinsicInst>(U);
+    Function *ParentFunc = Inst->getParent()->getParent();
+
----------------
Here too.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerIntrinsics.cpp:181
+
+    Function *Caller = CI->getParent()->getParent();
+    const AMDGPUSubtarget &ST = AMDGPUSubtarget::get(TM, *Caller);
----------------
You could replace the double `getParent()` by a single call to `getFunction()`




================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerIntrinsics.cpp:237
+
+      switch (Intrin->getIntrinsicID()) {
+      case Intrinsic::memcpy:
----------------
I think you can avoid the switch if you used the return value of `expandMemIntrinsic` and `makeLIDRangeMetadata`.

```
for (Function::iterator I = F.begin(), E = F.end(); I != E;) {
    BasicBlock *BB = &*I++;
    for (BasicBlock::iterator J = BB->begin(), JE = BB->end(); J != JE;) {
      IntrinsicInst *Intrin = dyn_cast<IntrinsicInst>(&*J++);
      if (!Intrin)
        continue;
      if (expandMemIntrinsic(Intrin, TTI)) {
          BB = Intrin->getParent();
          JE = BB->end();
          Intrin->eraseFromParent();
          Changed = true;
          continue;
       }
       Changed |= ST.makeLIDRangeMetadata(Intrin);
    }
}
```
However, that would also mark the range for the `amdgcn_workitem_id_xyz` intrinsics and I'm not sure if that's expected.


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

https://reviews.llvm.org/D138240