[PATCH] D123467: [NFC][CodeGen] Use ArrayRef in TargetLowering functions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2259
+  }
+  void setOperationAction(ArrayRef<unsigned> Ops, ArrayRef<MVT> VTs,
+                          LegalizeAction Action) {
----------------
Can't we have ArrayRef for both operands and use 2 for loops. Why do we need two functions?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:446
 
-  setOperationAction(ISD::FREM,    MVT::f16,   Promote);
-  setOperationAction(ISD::FREM,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FREM,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FPOW,    MVT::f16,   Promote);
-  setOperationAction(ISD::FPOW,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FPOW,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FPOWI,   MVT::f16,   Promote);
-  setOperationAction(ISD::FPOWI,   MVT::v4f16, Expand);
-  setOperationAction(ISD::FPOWI,   MVT::v8f16, Expand);
-  setOperationAction(ISD::FCOS,    MVT::f16,   Promote);
-  setOperationAction(ISD::FCOS,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FCOS,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FSIN,    MVT::f16,   Promote);
-  setOperationAction(ISD::FSIN,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FSIN,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FSINCOS, MVT::f16,   Promote);
-  setOperationAction(ISD::FSINCOS, MVT::v4f16, Expand);
-  setOperationAction(ISD::FSINCOS, MVT::v8f16, Expand);
-  setOperationAction(ISD::FEXP,    MVT::f16,   Promote);
-  setOperationAction(ISD::FEXP,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FEXP,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FEXP2,   MVT::f16,   Promote);
-  setOperationAction(ISD::FEXP2,   MVT::v4f16, Expand);
-  setOperationAction(ISD::FEXP2,   MVT::v8f16, Expand);
-  setOperationAction(ISD::FLOG,    MVT::f16,   Promote);
-  setOperationAction(ISD::FLOG,    MVT::v4f16, Expand);
-  setOperationAction(ISD::FLOG,    MVT::v8f16, Expand);
-  setOperationAction(ISD::FLOG2,   MVT::f16,   Promote);
-  setOperationAction(ISD::FLOG2,   MVT::v4f16, Expand);
-  setOperationAction(ISD::FLOG2,   MVT::v8f16, Expand);
-  setOperationAction(ISD::FLOG10,  MVT::f16,   Promote);
-  setOperationAction(ISD::FLOG10,  MVT::v4f16, Expand);
-  setOperationAction(ISD::FLOG10,  MVT::v8f16, Expand);
+  for (auto VT :
+       {ISD::FREM, ISD::FPOW, ISD::FPOWI, ISD::FCOS, ISD::FSIN, ISD::FSINCOS,
----------------
This for loop variable name doesn't make sense. They aren't VTs they're opcodes


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:627
     // Always expand sin/cos functions even though x87 has an instruction.
-    setOperationAction(ISD::FSIN   , MVT::f80, Expand);
-    setOperationAction(ISD::FCOS   , MVT::f80, Expand);
-    setOperationAction(ISD::FSINCOS, MVT::f80, Expand);
-
-    setOperationAction(ISD::FFLOOR, MVT::f80, Expand);
-    setOperationAction(ISD::FCEIL,  MVT::f80, Expand);
-    setOperationAction(ISD::FTRUNC, MVT::f80, Expand);
-    setOperationAction(ISD::FRINT,  MVT::f80, Expand);
-    setOperationAction(ISD::FNEARBYINT, MVT::f80, Expand);
-    setOperationAction(ISD::FMA, MVT::f80, Expand);
-    setOperationAction(ISD::LROUND, MVT::f80, Expand);
-    setOperationAction(ISD::LLROUND, MVT::f80, Expand);
-    setOperationAction(ISD::LRINT, MVT::f80, Custom);
-    setOperationAction(ISD::LLRINT, MVT::f80, Custom);
+    setOperationAction({ISD::FSIN, ISD::FCOS, ISD::FSINCOS, ISD::FFLOOR,
+                        ISD::FCEIL, ISD::FTRUNC, ISD::FRINT, ISD::FNEARBYINT,
----------------
There a logical grouping of sin/cos and conversion functions where the blank line was. This removes that grouping and now has a comment that only refers to the 3 of the things in this list.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:747
+                        ISD::SELECT_CC},
+                       VT, Expand);
     for (MVT InnerVT : MVT::fixedlen_vector_valuetypes()) {
----------------
I'm not sure this an improvement. It's one line longer and you have to look quite a bit further ahead to understand what it's doing. Maybe there's some middle ground of breaking this up into logical groups that would reduce the number of lines. Like putting all the CTPOIP/CTTZ/CTLZ in one group? @RKSimon what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123467