This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Add UnaryOperator visitor to SCCP
ClosedPublic

Authored by cameron.mcinally on Jun 3 2019, 11:24 AM.

Details

Summary

I don't know this pass at all, so would appreciate a thorough review.

I did notice that FP BinaryOperators are handled conservatively. FNeg is more an XOR than an FP operation, so I think I handled it appropriately. Please advise...

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 11:24 AM
craig.topper added inline comments.Jun 3 2019, 11:28 AM
llvm/lib/Transforms/Scalar/SCCP.cpp
996

Binary->Unary

Fix comment: Binary->Unary.

fhahn added a reviewer: fhahn.Jun 3 2019, 12:28 PM
fhahn added a subscriber: fhahn.

Thanks for adding this.

Please also add non-undef tests for unary operators, e.g. something like '%v = fneg double 10.0` or %v1 = fadd double 10.0, 20.0; %v2 = fneg double %v1.

llvm/lib/Transforms/Scalar/SCCP.cpp
975

nit: I think Op0V or something that makes it explicit that this ties back to the first operand of the instruction would be a slightly better name.

Rename VState to V0State as requested.

Also add constant folding test for unary fneg.

cameron.mcinally marked an inline comment as done.Jun 3 2019, 12:52 PM
cameron.mcinally marked an inline comment as done.
cameron.mcinally added inline comments.
llvm/lib/Transforms/Scalar/SCCP.cpp
975

Oh, just realized you requested Op0V instead of V0State. I copied that naming from visitBinaryOperator(...). Are you okay with this?

fhahn added inline comments.Jun 3 2019, 1:06 PM
llvm/test/Transforms/SCCP/apfloat-basictest.ll
4 ↗(On Diff #202780)

I think it would be better to explicitly check the return value, instead of relying on grep. Also, would it be possible to add another case where the operand to fneg itself needs to be propagated before we simplify fneg?

Add a test as suggested...

cameron.mcinally marked an inline comment as done.Jun 3 2019, 2:04 PM
fhahn accepted this revision.Jun 3 2019, 2:05 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jun 3 2019, 2:05 PM
fhahn added inline comments.Jun 3 2019, 2:07 PM
llvm/test/Transforms/SCCP/apfloat-basictest.ll
4 ↗(On Diff #202794)

Needs FileCheck now.

Add FileCheck...

cameron.mcinally marked 2 inline comments as done.Jun 3 2019, 2:38 PM
This revision was automatically updated to reflect the committed changes.