This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add initial sign analysis
ClosedPublic

Authored by martong on Oct 25 2022, 2:04 AM.

Details

Summary

This patch adds an initial implementation for sign analysis, with the
following lattice (T: top, N: negative, Z: zero, P: positive, B: bottom):

  T
/ | \
N Z P
\ | /
  B

The lattice is implemented with BoolValue properties attached to other
Values.

Diff Detail

Event Timeline

martong created this revision.Oct 25 2022, 2:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong requested review of this revision.Oct 25 2022, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 2:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
martong edited the summary of this revision. (Show Details)Oct 25 2022, 2:05 AM
ymandel accepted this revision.Oct 25 2022, 11:05 AM

Nice!! Just nits.

At a high level, I'm a little concerned about this as a demo, since I wouldn't recommend this implementation in practice (for various reasons, e.g. I would encode with 2 booleans since there are only 3 values). But, I think this approach has the advantage of clarity over optimized approaches. It might be worth pointing this out in comments at places where you made a decision for purposes of clarity/readability, if any come to mind.

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
63

please indicate ownership in comment

93

Please comment on the meaning of the tuple. Even better, define a (named) struct.
In general, this function could use a comment explaining what it does.

114

Please expand this out a bit. e.g. OperandProps. Opd is not a familiar/common abbreviation, so I think its use harms readability.

115

Given the boolean nature of these properties, it's really easy to misread these pointer operations as boolean operations, and be confused (at least, easy for me to be confused, so I'm being selfish :)).

135

FIXME

144

perhaps something more readable, like LHSProps or LeftProps?

146

please expand pointer operation (as above)

184

why not? its a test, so not necessary, just curious to know why you're leaving this out.

194

same comments as above about variable naming.

281
397

It doesn't appear to be used.

399

Given the assertion on line 400, I think this name shouldn't be First. Maybe Only, or Unique?

426

typo?

This revision is now accepted and ready to land.Oct 25 2022, 11:05 AM
martong marked 13 inline comments as done.Oct 26 2022, 6:08 AM

Nice!! Just nits.

At a high level, I'm a little concerned about this as a demo, since I wouldn't recommend this implementation in practice (for various reasons, e.g. I would encode with 2 booleans since there are only 3 values). But, I think this approach has the advantage of clarity over optimized approaches. It might be worth pointing this out in comments at places where you made a decision for purposes of clarity/readability, if any come to mind.

Okay, thanks again for the review! I've changed the comments for the file, where I describe these reasons about using 3 booleans.

clang/unittests/Analysis/FlowSensitive/SignAnalysisTest.cpp
184

This was my thought process: There is nothing that we can say about b if a is positive. Because b can still be either negative, positive, or even zero. Similar reasoning goes to the case when a is negative.

However, we could have an implication if a is zero, namely that b is either positive or negative. But I think implications with or are not useful, they should be then rather expressed in the lattice. So, we should rather have a new boolean value like PosOrNeg, but I've found that too complicated for this demo.

397

Yes, good catch, it's been left here for some older version of the implementation. I removed it now.

martong updated this revision to Diff 470796.Oct 26 2022, 6:08 AM
martong marked 2 inline comments as done.
  • Address ymandel's comments
This revision was landed with ongoing or failed builds.Oct 26 2022, 6:20 AM
This revision was automatically updated to reflect the committed changes.