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

[Andrew Savonichev via Phabricator via llvm-commits]

asavonic added inline comments.


================
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)
----------------
dmgreen wrote:
> Can this use haveNoCommonBitsSet, similar to ARMDAGToDAGISel::SelectAddLikeOr?
Done.


================
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);
----------------
dmgreen wrote:
> Do we need to check both operands? The Add/Or should be canonicalized to have the constant on the RHS.
Thanks. I was not sure that we can expect that.
Removed the extra check.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:15599
+
+  LLVM_DEBUG(dbgs() << "CombineBaseUpdate: trying "; N->dump());
+  LLVM_DEBUG(dbgs() << "CombineBaseUpdate: addr   "; Addr->dump());
----------------
dmgreen wrote:
> 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.
Let's remove them.
They were useful for debugging, but standard messages from DAG combiner are good enough.



================
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
----------------
dmgreen wrote:
> How come this test is changing?
Oh, this one is tricky.
The original test expected to see an OR instruction when the stack is aligned, and an ADD instruction when it is not. After the patch both ADD and OR are folded with loads/stores, so the two sequences (test1 and test2) are completely identical.

I changed the IR slightly, so that OR is not folded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108988