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

[Thomas Symalla via Phabricator via llvm-commits]

tsymalla added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:2
+//=== lib/CodeGen/GlobalISel/AMDGPUPostLegalizerCombiner.cpp
+//---------------===//
 //
----------------
arsenm wrote:
> This has too many -s then
I reverted this change.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp:285
+
+  if (!mi_match(Base, MRI, m_GISelect(m_GICmp(m_Pred(Predicate2), m_Reg(), m_Reg()), m_Reg(MatchInfo.AssignValue), m_ICst(MatchInfo.Cmp2))))
+    return false;
----------------
arsenm wrote:
> This should still be trying to match against the min/max opcodes, not looking for compare and selects
So, if I understand this correctly:

- The select/compare pattern I'm trying to match here will be matched to G_MIN/G_MAX
- This seems to be not implemented yet or not working for my case (as far as I can see that)
 
I'm not sure what should be achieved here. Should there either be a "future-proof" implementation for matching min and max which is not working yet, an additional case for matching min and max which falls back  to my current implementation or a combination of a) extension of the pattern matching that combines select and icmp to max / min and b) an implementation which only matches max / min and combines it accordingly?
Would you mind explaining this to me a little bit more?


================
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
+
----------------
arsenm wrote:
> tsymalla wrote:
> > arsenm wrote:
> > > I dislike anonymous values in tests, can you add names here
> > Hi, what do you mean by anonymous values in this context?
> The numbered %0, %1. They should have fixed string names instead. opt -S -instnamer will do this for you or you can manually %name them
Thanks. I manually named them.


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

https://reviews.llvm.org/D93708