[PATCH] D139420: [AArch64][GlobalISel] implement GPR (U/S)(MIN/MAX) instr support

[Amara Emerson via Phabricator via llvm-commits]

aemerson accepted this revision.
aemerson added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:194-202
+      .legalIf([=](const LegalityQuery &Query) {
+                 if (HasCSSC && typeInSet(0, {s32, s64})(Query))
+                   return true;
+                 return typeInSet(0, {v8s8, v16s8, v4s16, v8s16, v2s32, v4s32})
+                     (Query);
+               })
+      .minScalarIf([=](const LegalityQuery &Query) {
----------------
stuij wrote:
> arsenm wrote:
> > stuij wrote:
> > > arsenm wrote:
> > > > You can also conditionally add the predicates based on the subtarget feature check. i.e. you don't need to check the predicates inside the lambda, you can append rules for different subtargets. 
> > > Right. Indeed, I didn't think of that. Reviewing I still prefer the lambda-inlining. Maybe because I'm giddy to finally have an excuse to write lots of lambdas in C++. From your phrasing I think you're also ok with the current setup.
> > These are called a lot 
> If you're happy with this, I'll do the same to the other outstanding patches, lambda's be damned.
I think Matt's right in general, although IMO if it becomes extremely unwieldy for some reason we can embed the checks into the predicates (its ultimately not a big deal with these uncommon operations).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139420