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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
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;
----------------
tsymalla wrote:
> 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?
The compare+select pattern you are matching here should be matched separately into the min/max opcodes as a better, more canonical form. This is not implemented in GlobalISel yet.

Matching min/max should be implemented in GlobalISel, however I don't think this is a priority. Theoretically the IR optimizers should be matching these in the vast majority of cases with the new min/max intrinsics. A GlobalISel combine would theoretically be useful if the compare/select pattern were to be emitted by independent legalizations, but I'm currently not aware of any implemented case where this could happen


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

https://reviews.llvm.org/D93708