This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Add CHECK lines for unordered FP reductions
ClosedPublic

Authored by kmclaughlin on May 24 2021, 6:02 AM.

Details

Summary

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.

Diff Detail

Event Timeline

kmclaughlin requested review of this revision.May 24 2021, 6:02 AM
kmclaughlin created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2021, 6:02 AM
sdesmalen added inline comments.May 24 2021, 6:35 AM
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. :(

kmclaughlin marked 3 inline comments as done.
  • 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?

kmclaughlin marked 3 inline comments as done.
  • Renamed some confusing variable names in the fadd_strict_interleave tests
david-arm accepted this revision.May 25 2021, 7:56 AM

LGTM! Thanks for making the changes. :)

This revision is now accepted and ready to land.May 25 2021, 7:56 AM
This revision was automatically updated to reflect the committed changes.