[PATCH] D52653: [CodeGen, AArch64] Combine Interleaved Loads which are not covered by the Vectorizer

[Martin Elshuber via Phabricator via llvm-commits]

marels added a comment.

@john.brawn thanks for the input. I commented on some and will upload a new revision shortly



================
Comment at: lib/CodeGen/InterleavedLoadCombinePass.cpp:225-226
+
+  /// Value
+  Value *V;
+
----------------
marels wrote:
> john.brawn wrote:
> > I don't think this is needed. hasB() can check !B.empty(), and in isCompatibleTo() we can assume that the user of this class will be using polynomials of the same variable.
> I think it is necessary. The vector B stores the operations executed on V, but V is the value those operations are applied on. Example:
> 
> ```
> define @fn(i64 %a, i64 %b, u8 *%base)
> %ptr1 = gep %base, %a 
> %ptr2 = gep %base, %b
> ```
> 
> This leads to two incompatible polynomials:
> ```
> 1*%a + 0 and 
> 1*%b + 0```
> whereas:
> ```
> Pa = { .B = <{mul, 1}>, V = %a, A = 0}
> ```
> and
> ```
> Pb = { .B = <{mul, 1}>, V = %b, A = 0}
> ```
> 
> Currently `{mul, 1}` is stored in `B` thus `B.empty()` could be used. But this is not necessary but pushing {mul, 1} unnecessarily restricts the computation, and i think I will remove this.
> 
> 
> 
> 
To clarify semantics:
  - hasB() -> means Is the first-order term zero
  - B.empty() -> means The first-order term is multiplied by one.
I think renaming the function to isFirstOrder makes this semantics more clearly. 


Repository:
  rL LLVM

https://reviews.llvm.org/D52653