[PATCH] D46273: [InstCombine, ARM] Convert vld1 to llvm load

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:932
+/// from a constant, since we get constant-folding for free.
+static Value *simplifyVectorLoad(const IntrinsicInst &II,
+                                 unsigned MemAlign,
----------------
The name makes this look like a generic helper, but it's only applicable to neon, right? 

Why is it sitting in the middle of a pile of x86 code?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:935
+                                 InstCombiner::BuilderTy &Builder) {
+  auto IntrAlign = dyn_cast<ConstantInt>(II.getArgOperand(1));
+  unsigned Alignment = IntrAlign && IntrAlign->getZExtValue() < MemAlign ?
----------------
labrinea wrote:
> javed.absar wrote:
> > There is a check of IntrAlign, but then IntrAlign is still used otherwise? Maybe it will be cleaner to not use 'auto'
> Using auto as it's inferred by the dynamic cast.
LLVM preference is to use 'auto *' for pointers:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:936-937
+  auto IntrAlign = dyn_cast<ConstantInt>(II.getArgOperand(1));
+  unsigned Alignment = IntrAlign && IntrAlign->getZExtValue() < MemAlign ?
+                       MemAlign : IntrAlign->getZExtValue();
+
----------------
This doesn't make sense when IntrAlign is not a ConstantInt. Please add a test like this:

```
define <2 x i64> @vld1_2x64(i8* %ptr, i32 %x) {
  %vld1 = call <2 x i64> @llvm.arm.neon.vld1.v2i64.p0i8(i8* %ptr, i32 %x)
  ret <2 x i64> %vld1
}

```
...or if that's not supposed to be allowed, then we don't need a dyn_cast.


https://reviews.llvm.org/D46273