[PATCH] D123347: [ISD::IndexType] Helper functions for common queries.

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:1377
+
+inline MemIndexType getUnscaledIndexType(MemIndexType IndexType) {
+  return isIndexTypeSigned(IndexType) ? SIGNED_UNSCALED : UNSIGNED_UNSCALED;
----------------
For symmetry is it worth having a `getScaledIndexType` too that returns SIGNED_SCALED or UNSIGNED_SCALED?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:10457
     SDValue Op = Index.getOperand(0);
-    MGS->setIndexType(Scaled ? ISD::UNSIGNED_SCALED : ISD::UNSIGNED_UNSCALED);
     if (TLI.shouldRemoveExtendFromGSIndex(Op.getValueType())) {
----------------
Removing this looks like an actual function change, not just refactoring. It looks sensible, but it's not obvious from the commit message that you're also changing the behaviour. After your patch we won't change the index type unless we manage to refine the index.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4706
 
-  bool IsScaled =
-      IndexType == ISD::SIGNED_SCALED || IndexType == ISD::UNSIGNED_SCALED;
-  bool IsSigned =
-      IndexType == ISD::SIGNED_SCALED || IndexType == ISD::SIGNED_UNSCALED;
+  bool IsScaled = MGT->isIndexScaled();
+  bool IsSigned = MGT->isIndexSigned();
----------------
Given you already have the IndexType as a variable, can you just call the ISD function helper functions instead? For example,

```  bool IsScaled = ISD::isIndexTypeScaled(IndexType);
  bool IsSigned = ISD::isIndexTypeSigned(IndexType);```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4805
 
-  bool IsScaled =
-      IndexType == ISD::SIGNED_SCALED || IndexType == ISD::UNSIGNED_SCALED;
-  bool IsSigned =
-      IndexType == ISD::SIGNED_SCALED || IndexType == ISD::SIGNED_UNSCALED;
+  bool IsScaled = MSC->isIndexScaled();
+  bool IsSigned = MSC->isIndexSigned();
----------------
Same comment as before about using the ISD helper functions directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123347