This is an archive of the discontinued LLVM Phabricator instance.

Fix SLPVectorizer commutativity reordering
ClosedPublic

Authored by mehdi_amini on Oct 22 2015, 2:36 PM.

Details

Summary

Should fix https://llvm.org/bugs/show_bug.cgi?id=25247

Still a draft, I'll update with a test case but I'd like a first feedback on the change.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Fix SLPVectorizer commutativity reordering.
mehdi_amini updated this object.
mehdi_amini added a reviewer: mzolotukhin.
mehdi_amini added subscribers: llvm-commits, dexonsmith.

Fix a typo (should have ran the test-suite first)

mzolotukhin edited edge metadata.Oct 22 2015, 2:54 PM

Hi Mehdi,

Would it be possible to split it to two patches: one with factoring out shouldReorderOperands function, and the other with functional change? It would be easier to review (No worries if it's not possible).

Thanks,
Michael

mehdi_amini edited edge metadata.

Patch update after NFC refactoring applied to trunk.

mzolotukhin accepted this revision.Oct 23 2015, 12:10 PM
mzolotukhin edited edge metadata.

Hi Mehdi,

The change looks good to me provided a test would be added.

Thanks,
Michael

lib/Transforms/Vectorize/SLPVectorizer.cpp
2003–2012

nitpick: Symmetrically with double 'm'?

This revision is now accepted and ready to land.Oct 23 2015, 12:10 PM
mehdi_amini updated this revision to Diff 39307.Nov 4 2015, 7:25 PM
mehdi_amini edited edge metadata.

Added a test, can you double check?

Last diff is not the right one, sorry I'll fix this soon.

Looks like I don't know how to use Phab... Patch is ready :)

Hi Mehdi,

As before, the code looks good to me, some comments on the test are inline.

Thanks,
Michael

test/Transforms/SLPVectorizer/X86/commutativity.ll
8–9

Isn't the second check (CHECK-NOT) redundant here? How is it possible, that we have store <16 x i8> and one more store after it?

18–19

As far as I understood, the test checks if we can reorder operands to create a broadcasted operand. Do we need (or do we have already) a separate test, that will check that we try to commute to get 'same opcode' operands?

Like:

%mul1 = mul ...
%mul2 = mul ...
%sub = sub ...
%div = div ...

%x = add %sub, %mul1  ; <<< Need to reorder commutative operands to get
%y = add %mul2, %div  ; <<< MUL at the same position for %x and %y

store %x
store %y
mehdi_amini updated this revision to Diff 39535.Nov 6 2015, 8:00 AM

Improve test: check for operand reordering

Hi Mehdi,

Thanks for adding the tests! LGTM modulo a couple of nitpicks.

Michael

test/Transforms/SLPVectorizer/X86/commutativity.ll
2

Should we do opt < %s instead of opt %s to avoid overwriting the input file?

60–61

Just noticed several typos here (and in the same comment in the previous test):
s/detects/detect/
s/levaraging/leveraging/
s/propriety/property/

mehdi_amini added inline comments.Nov 6 2015, 10:02 AM
test/Transforms/SLPVectorizer/X86/commutativity.ll
2

I don't think it would ever overwrite the input file unless you force it to do so explicitly. But Duncan had a point: the ModuleID would be patch sensitive without the redirection.

60–61

Thanks, will update.

Update test before commit

mehdi_amini closed this revision.Nov 16 2015, 8:42 AM