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

Wed Dec 23 07:15:24 PST 2020

arsenm added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h:475
+template <typename Boundary1, typename Boundary2, typename Origin>
+struct MaxMin_match_helper {
+  Boundary1 B1;
----------------
This is doing too much, this should just match the G_UMIN/G_UMAX/G_SMIN/G_SMAX opcodes. This shouldn't be responsible for matching the compare+select pattern

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll:1
+; RUN: llc -global-isel -mcpu=tahiti -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs < %s | FileCheck --check-prefixes=GFX678,GFX6789 %s
+; RUN: llc -global-isel -mcpu=gfx900 -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs < %s | FileCheck --check-prefixes=GFX9,GFX6789 %s
----------------
I would prefer if the test name had a more consistent name. How about combine-int-clamp.ll?

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll:18-21
+  %2 = icmp slt i64 %1, 32767
+  %3 = select i1 %2, i64 %1, i64 32767
+  %4 = trunc i64 %3 to i16
+
----------------
I dislike anonymous values in tests, can you add names here

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93708