[PATCH] D68337: [ARM][MVE] Enable extending masked loads

[Sam Parker via Phabricator via llvm-commits]

samparker marked an inline comment as done.
samparker added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9283
+                                        ISD::NodeType ExtOpc) {
+  if (!TLI.isLoadExtLegal(ExtLoadType, VT, N0.getValueType()))
+    return SDValue();
----------------
dmgreen wrote:
> samparker wrote:
> > dmgreen wrote:
> > > Is it true that whenever you have a legal extending load, you will also have the equivalent legal extending masked load? (For MVE we do, but is that true for all archs?)
> > > 
> > > Do we need to add an extra set of flags for this? Or is isVectorLoadExtDesirable good enough to handle these cases when there is an asymmetry?
> > Yes, we can't expect that it's true for everything. I don't understand why the APIs generally like to pass lots of arguments instead of just passing, say the load that you'd want to inspect... So hopefully both these calls will cover all cases and I'd like to avoid adding another flag. That or I could just change isLoadExtLegal to take the LoadSDNode, but I've assumed these calls are designed like they are for reason...
> They refer back to the LoadExtActions, which are set by setLoadExtAction in ISel. We may need more flags on there to specify the difference between the masked loads and the normal loads.
I had a look and I don't see how we could add an extra flag here. isLoadExtLegal will return true if the operation has been marked as legal, but for both targets (arm, x86) the MLOAD and MSTORE operations are set as custom. So I think having both calls will be necessary, with isVectorLoadExtDesirable enabling the fine grained control that we need.


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

https://reviews.llvm.org/D68337