[PATCH] D39415: [ARMISelLowering] Better handling of NEON load/store for sequential memory regions

[Javed Absar via Phabricator via llvm-commits]

javed.absar added a comment.

Hi Eugene:
Thanks for the work. However, in its current form the implementation is bit hard to follow. Would it be possible to, describe -

1. Your overall approach in the implementation

2.. Describe above the function what it is trying to do (e.g. checkedGetIncrement - which b.t.w looks non-intuitive name).

You do have comments everywhere but it is hard to follow the string of thought in its current form.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10996
 
+static bool isUpdatingVLDorVST(SDNode *Inst) {
+  switch(Inst->getOpcode()) {
----------------
But you are checking only VLD1/VST1, so you may want to change function name appropriately. VLD1OrVST1
...................................^^         ^


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11006
+
+static ConstantSDNode *tryGetConstOperand(SDNode *Inst, unsigned NOp) {
+   return dyn_cast<ConstantSDNode>(Inst->getOperand(NOp).getNode());
----------------
Seems unnecessary as single use


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11065
+  while (true) {
+    for (SDNode::use_iterator UI = Addr.getNode()->use_begin(),
+           UE = Addr.getNode()->use_end(); UI != UE; ++UI) {
----------------
nitpick. Better to use range loop


Repository:
  rL LLVM

https://reviews.llvm.org/D39415