An additional RUN line has been added to both strict-fadd.ll &
scalable-strict-fadd.ll to ensure the correct behaviour of these
tests where -enable-strict-reductions is false.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/Transforms/LoopVectorize/AArch64/scalable-strict-fadd.ll | ||
---|---|---|
214 | Can you add the PHI node for the reduction? (same for any other tests that miss this) |
I think the patch looks good to me, but I'll hold off a LGTM for now since I justed realise you've got other comments too.
llvm/test/Transforms/LoopVectorize/AArch64/scalable-strict-fadd.ll | ||
---|---|---|
136 | nit: It looks like none of the variables in the vector.ph is actually used further down. Maybe it's possible to just delete the whole vector.ph block since it's tested in ORDERED case anyway, unless you're now going to add PHIs to the vector.body which show INDUCTION being used? | |
195–196 | nit: I realise you've not changed the test name here, but I think maybe the name is a little confusing because there isn't actually anything invariant in the loop itself. Although I do realise this probably came from C code where an invariant condition was being tested in the loop and got hoisted out. I think what's actually being tested here is that we can vectorise loops with normal fadds feeding into a reducing fadd, i.e. for (i = 0; i < n; i++) { r += a[i] + b[i]; } Maybe it's worth changing the name as part of this patch to something like fadd_of_fadds since it's still NFC? | |
llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll | ||
270–271 | nit: Same comment as in the scalable test. :( |
- Added CHECK-UNORDERED lines for PHI nodes to all tests which were missing this
- Renamed the fadd_invariant tests to fadd_of_sum
llvm/test/Transforms/LoopVectorize/AArch64/scalable-strict-fadd.ll | ||
---|---|---|
136 | These weren't being used, though I've kept them here now that I've added check lines for the PHI nodes that use LOAD1/LOAD2 |
Hi @kmclaughlin I think the latest patch looks good now apart from a few minor comments about possibly confusing variable names in a couple of tests!
llvm/test/Transforms/LoopVectorize/AArch64/scalable-strict-fadd.ll | ||
---|---|---|
137 | nit: Maybe this should be named INS_ELT2 to match LOAD2? | |
144 | nit: Again, I think maybe here it looks better as VEC_PHI2 and INS_ELT2 to match the second load/reduction variable? And similarly for the PHI below? | |
llvm/test/Transforms/LoopVectorize/AArch64/strict-fadd.ll | ||
218 | nit: I think there is a similar naming scheme issue to the scalable equivalent? |
nit: It looks like none of the variables in the vector.ph is actually used further down. Maybe it's possible to just delete the whole vector.ph block since it's tested in ORDERED case anyway, unless you're now going to add PHIs to the vector.body which show INDUCTION being used?