|4011 ↗||(On Diff #201613)|
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.
|4001 ↗||(On Diff #201613)|
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?
|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.
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.