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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-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;
----------------
david-arm wrote:
> For symmetry is it worth having a `getScaledIndexType` too that returns SIGNED_SCALED or UNSIGNED_SCALED?
D123381 removes the scaling property so it didn't seem worth adding an unused function only to later remove it.


================
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())) {
----------------
david-arm wrote:
> 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.
The new else block should maintain the original behaviour?  The only difference it that is doesn't overwrite `MSG`'s IndexType with the same value it already has.


================
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();
----------------
david-arm wrote:
> 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);```
D123381 removes the scaling property and so querying `MGT` directly becomes the only way to know if the index is scaled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123347