[PATCH] D108988: [ARM] Simplify address calculation for NEON load/store

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Nice work. I'm glad this works this way.



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:15522
+    // pointer is larger than the value of the constant operand.
+    FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Ptr);
+    if (!FI)
----------------
Can this use haveNoCommonBitsSet, similar to ARMDAGToDAGISel::SelectAddLikeOr?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:15540
+  case ISD::OR: {
+    if (auto *CIncOp0 = dyn_cast<ConstantSDNode>(N->getOperand(0).getNode())) {
+      *Ptr = N->getOperand(1);
----------------
Do we need to check both operands? The Add/Or should be canonicalized to have the constant on the RHS.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:15599
+
+  LLVM_DEBUG(dbgs() << "CombineBaseUpdate: trying "; N->dump());
+  LLVM_DEBUG(dbgs() << "CombineBaseUpdate: addr   "; Addr->dump());
----------------
How useful are these error messages do you think, in the long run?

The number of combines tried can be quite high, and these may just end up adding noise for people not looking at CombineBaseUpdate combines. It seems to be fairly uncommon to add debug messages for DAG combines.

But if you think they are useful, feel free to keep them around.


================
Comment at: llvm/test/CodeGen/ARM/alloc-no-stack-realign.ll:19
  %retval = alloca <16 x float>, align 64
- %0 = load <16 x float>, <16 x float>* @T3_retval, align 16
- store <16 x float> %0, <16 x float>* %retval
- %1 = load <16 x float>, <16 x float>* %retval
- store <16 x float> %1, <16 x float>* %agg.result, align 16
+ %a1 = bitcast <16 x float>* %retval to float*
+ %a2 = getelementptr inbounds float, float* %a1, i64 8
----------------
How come this test is changing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108988