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

Amara Emerson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 5 00:32:02 PST 2023


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



More information about the llvm-commits mailing list