[PATCH] D42838: [AMDGPU] added writelane intrinsic

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Thank you for taking another look. I understand now, and I agree that there doesn't seem to be a super clean solution. Not to mention that the function prolog/epilog code here is clearly incorrect, because it is saving/restoring the VGPR with a potentially insufficient EXEC mask.

Anyway, those issues are unrelated, and I think your approach to solving it is reasonable now. I'd just ask you to inline that one helper function as mentioned in the comment, apart from that the change looks good to me.



================
Comment at: lib/Target/AMDGPU/SIMachineFunctionInfo.h:656-664
+  // During SGPR spilling to VGPR, determine if the VGPR is defined. The only
+  // circumstance in which we say it is undefined is when it is the first spill
+  // to this VGPR in the first basic block. This function relies on being called
+  // in code order in the first basic block by the SGPR spilling code.
+  bool isSGPRSpillVGPRDefined(const MachineBasicBlock *MBB, unsigned VGPR) {
+    if (&MBB->getParent()->front() != MBB)
+      return true; // not first basic block
----------------
I don't like this function. The name is misleading: it suggests a pure function ("is XXX"), but in reality there is a side effect; and the list of parameters is pretty surprising given the name as well.

Please just inline the function (including its comments) in the one single location where it is used, I think that will be much cleaner.


Repository:
  rL LLVM

https://reviews.llvm.org/D42838