Page MenuHomePhabricator

[CFLGraph] Add support for unary fneg instruction.
ClosedPublic

Authored by craig.topper on Jun 1 2019, 11:33 PM.

Details

Summary

I don't know much about this analysis so I'm not sure the best way to test this.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Jun 1 2019, 11:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2019, 11:33 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

I don't know much about this pass either, but it looks like test/Analysis/CFLAliasAnalysis is the right place.

I hacked up Steensgaard/constant-over-index.ll to include an fneg and that seems to exercise the code. Your guess is better than mine if it's sufficient coverage or not.

--- a/llvm/test/Analysis/CFLAliasAnalysis/Steensgaard/constant-over-index.ll
+++ b/llvm/test/Analysis/CFLAliasAnalysis/Steensgaard/constant-over-index.ll
@@ -18,7 +18,9 @@ loop:
 
   %p.0.i.0 = getelementptr [3 x [3 x double]], [3 x [3 x double]]* %p, i64 0, i64 %i, i64 0
 
-  store volatile double 0.0, double* %p3
+  %neg = fneg double 0.0
+
+  store volatile double %neg, double* %p3
   store volatile double 0.1, double* %p.0.i.0

To be clear, I'm not suggesting that as a test -- just proving that the new code is exercised with this change.

Thanks you both! LGTM with a test, assuming that adding the test is straightforward.

Similar comments as on D62790: presuming that fneg doesn't operate on pointers, this code will just be a nop. If CFL* gets revived someday, we might end up trying to extract useful information from the flow of values through non-pointer-types, but as it stands, we just treat any inttoptr as magic and are conservative about it.

I suppose that if we wanted to be more explicit about this, we could have add*Edge return whether or not an edge was added, and just assert(!add*Edge(...)) for all FP ops, but that's out of the scope of this patch IMO.

it looks like test/Analysis/CFLAliasAnalysis is the right place

+1. CFLGraph bits are shared between both the Andersen/ and Steensgaard/ subdirectories, so it shouldn't make a difference which one we place the coverage line in.

just proving that the new code is exercised with this change

For the reasons noted above, "it doesn't crash" is probably the best test we can have here at the moment.

Stuck an fneg into an existing test that already had an fmul. This executes the visitUnaryOperator function. The constant expr path is still untested. But I think all the binary ops are untested there as well.

Fair enough; thanks again!

This revision is now accepted and ready to land.Jun 6 2019, 10:38 AM
This revision was automatically updated to reflect the committed changes.