[PATCH] D84644: AMDGPU/GlobalISel: Handle llvm.amdgcn.ds.{fadd|fmin|fmax}

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3227
+
+bool AMDGPULegalizerInfo::legalizeDSAtomicFPIntrinsic(LegalizerHelper &Helper,
+                                                      MachineInstr &MI,
----------------
madhur13490 wrote:
> "Helper" can be marked as "const"?
It wouldn't be if it were used as normal


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3234
+  MI.setDesc(ST.getInstrInfo()->get(getDSFPAtomicOpcode(IID)));
+  MI.RemoveOperand(1);
+
----------------
madhur13490 wrote:
> You could probably push this inside loop, run loop from 1 to 5 and skip 2 with a comment. That way it would be readable. If its possible then an uptick loop, please.
removing an operand in the middle of an instruction will force the instruction to shift all of its operands down each time, so it's much less efficient


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.fadd.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=tonga -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=GFX8 %s
+; RUN: llc -global-isel -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -enable-var-scope -check-prefix=GFX9 %s
----------------
madhur13490 wrote:
> Why just mesa3d? No implication on hsa?
The triple is irrelevant, but we error on the graphics shader conventions for HSA. We use the shaders for the simplest way to get sample SGPRs


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

https://reviews.llvm.org/D84644