[PATCH] D93708: [AMDGPU] Add a new Clamp Pattern to the GlobalISel Path.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Do we do the equivalent combine in the DAG path? I don't recognize this one



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:134
+
+  constexpr unsigned int CvtOpcode = AMDGPU::V_CVT_PK_I16_I32_e64;
+  assert(MI.getOpcode() != CvtOpcode);
----------------
tsymalla wrote:
> arsenm wrote:
> > It would be better to use a generic operation pre-regbank select rather than forcing register classes here
> How would I go for this?
You can follow the example of the other G_AMDGPU_* pseudos (e.g. G_AMDGPU_CVT_UBYTE0). I'm a bit ambivalent on whether we should do this in this case, as there are still some unanswered questions about how regbankselect should work. However, so far we've been using pseudos like this


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:71
+
+  LLVM_DEBUG(dbgs() << "Matching Clamp i64 to i16\n");
+
----------------
Probably shouldn't print here especially when failure is likely. Ideally we would get a message from the generated combiner based on the defined combine name


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:132
+
+  const auto REG_CLASS = &AMDGPU::VGPR_32RegClass;
+
----------------
I'd rather just repeat the VGPR_32RegClass (also shuoldn't use macro-like naming convention)


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/combine-short-clamp.ll:113
+}
\ No newline at end of file

----------------
Fix this whitespace error


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

https://reviews.llvm.org/D93708