Page MenuHomePhabricator

[IROutliner] Using Canonical Values to find Corresponding Items between Regions
ClosedPublic

Authored by AndrewLitteken on Aug 24 2021, 12:55 PM.

Details

Summary

D104143 introduced canonical value numbering between regions, which allows for the easy identification of items across a region, eliminating the need in the outliner to create parallel lists of instructions for each region, and replace output values in a less convoluted way,

Diff Detail

Event Timeline

AndrewLitteken requested review of this revision.Aug 24 2021, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2021, 12:55 PM
jroelofs added inline comments.Aug 24 2021, 1:06 PM
llvm/include/llvm/Transforms/IPO/IROutliner.h
166

I'd drop the Optional<> and use nullptr to represent "didn't find one". That removes ambiguity around Optional<Value*>(nullptr), and would improve the call sites.

llvm/lib/Transforms/IPO/IROutliner.cpp
1145–1152
1159

... to fix the dereference after dyn_cast without checking it for null.

1165
paquette added inline comments.Sep 2 2021, 2:14 PM
llvm/include/llvm/Transforms/IPO/IROutliner.h
166

+1

Updating function return type, and other comments

paquette added inline comments.Sep 7 2021, 2:52 PM
llvm/lib/Transforms/IPO/IROutliner.cpp
173

I think you can return FoundValueOpt.getValueOr(nullptr).

paquette accepted this revision.Sep 8 2021, 10:23 AM

This looks okay with nits.

I think the commit message can be more descriptive here though. It should explain why you're changing behaviour surrounding PHIs/stores etc.

llvm/lib/Transforms/IPO/IROutliner.cpp
1147

https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

Seems like it's more appropriate to use cast here than dyn_cast, considering you assert right after.

1148
This revision is now accepted and ready to land.Sep 8 2021, 10:23 AM
This revision was landed with ongoing or failed builds.Sep 8 2021, 11:36 AM
This revision was automatically updated to reflect the committed changes.