This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorize] Add FNeg instruction support
ClosedPublic

Authored by craig.topper on May 27 2019, 10:51 PM.

Event Timeline

craig.topper created this revision.May 27 2019, 10:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2019, 10:51 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

FYI that I have an incoming patch that will help make this one more concise. Will have a Diff up in the next hour or so...

Worth adding the test with current codegen so this patch shows the delta?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4013

Just posted D62521: Add CreateFNegFMF(...) to the IRBuilder. That should allow you to skip the explicit FMF copy.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4013

Oh, I now see that the BinOperator code above is the generic form too. I don't think there are any other unary operators planned, but it's probably best to keep the UnaryOperator code in line with the BinaryOperator code anyway.

fhahn added inline comments.May 28 2019, 9:48 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
4003

The duplication with the binary ops seems a little unfortunate to me. The widening is basically the same, except the number of arguments.

What would it take to make it generic to support both binary & unary ops? If we would have a builder function CreateNAryOp(Opcode, ArrayRef<Value*>) the generic version would be quite compact I think. The casts to UnaryOperator (and BinaryOperator) should not be necessary, right? For the copyIRFlags case, we only care if VecOp is an instruction?

llvm/test/Transforms/LoopVectorize/X86/fneg.ll
1 ↗(On Diff #201613)

Can you make the test independent of X86? I think you can just pass -force-vector-width=4, as we just want to test the widening, not anything cost-modeling related.

Move the test. I'll pre-commit it if we're satisfied.

I'll put together a follow up patch to merge the binary and unary operator handling

@fhahn does the test look ok to pre-commit now?

fhahn added a comment.May 29 2019, 3:14 PM

@fhahn does the test look ok to pre-commit now?

Personally I think the auto-generated assertions here do not really add much value, as they contain plenty of unrelated stuff and make the test more fragile than necessary. IMO it would be better to focus the check on the relevant bits: ensure we generate load -> fneg -> store in the vector body. But if you think the complete checks are more valuable, please commit the test as is, I do not have too strong feelings about it.

For some reason I missed the changes to LoopVectorizationCostModel::getInstructionCost. For those, a target-specific test would be good. I think most LV tests for that check the cost printed in the debug messages.

-Rebase after committing tests.
-Merge in D62534

Use range-based for loop as suggested in the other review.

Add comment to IRBuilder function.

fhahn accepted this revision.May 30 2019, 8:52 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 30 2019, 8:52 AM
This revision was automatically updated to reflect the committed changes.