Page MenuHomePhabricator

[DAG] Add initial SelectionDAG::isGuaranteedNotToBeUndefOrPoison framework (PR51129)

Authored by RKSimon on Jul 23 2021, 7:45 AM.



I've setup the basic framework for the isGuaranteedNotToBeUndefOrPoison call and updated DAGCombiner::visitFREEZE to use it, further Opcodes can be handled when we have test coverage.

I'm not aware of any vector test freeze coverage so the DemandedElts (and the Depth) args are not being used yet - but they are in place.

Diff Detail

Event Timeline

RKSimon created this revision.Jul 23 2021, 7:45 AM
RKSimon requested review of this revision.Jul 23 2021, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2021, 7:45 AM
aqjune added inline comments.Jul 23 2021, 8:45 AM

Probably relying on noundef attributes from library functions' return values will be helpful.
Does SelDag allow getting the function declaration from a call?

Also, a pointer that is dereferenced by load/store is also noundef.

RKSimon added inline comments.Jul 23 2021, 9:02 AM

Yes all of that should be viable - I just don't want to clog this initial version. Should I add specific TODO comments?

e.g. I'm intending to look at whether this can help with some of the regressions in D106675 - which would mean start adding vector coverage.

aqjune added inline comments.Jul 23 2021, 9:21 AM

Oh, yes. TODO comment would be fine.

RKSimon updated this revision to Diff 361270.Jul 23 2021, 10:20 AM

Add some TODO comments indicating the likely next steps after this initial commit

aqjune accepted this revision.Jul 23 2021, 12:29 PM

To me, this patch looks good (since it contains basic checks only), but I'd appreciate comments from other reviewers because I'm not pretty familiar with SelDag.

This revision is now accepted and ready to land.Jul 23 2021, 12:29 PM
efriedma accepted this revision.Jul 23 2021, 1:53 PM



I think I'd like to have a separate isGuaranteedNotToBePoison entry point, so code consuming this API doesn't have to explicitly pass the "PoisonOnly" flag. (Sharing the implementation is fine.)

Doesn't have to be in this patch, though.


I'd like to figure out our plan for this at some point but I guess adding one more API with the same restriction isn't a big deal.

This revision was landed with ongoing or failed builds.Jul 24 2021, 3:38 AM
This revision was automatically updated to reflect the committed changes.