[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: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:
> 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
So, what would be the right approach to handle this in your opinion? As far as I understand it doesn't make sense to have an implementation for this specific case where a generic implementation would be more suitable. Would you say it makes more sense to wait for the max / min matching in GlobalISel and implement my combine around it?


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

https://reviews.llvm.org/D93708