Several visitors check if operands to the instruction are constants, either as it is or after looking up SimplifiedValues, check if the result is a constant and update the SimplifiedValues map. This refactoring splits it into a common function that does the checking of whether the operands are constants and updating of the SimplifiedValues table, and an instruction specific part that is implemented by each instruction visitor as a lambda and passed to the common function.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Analysis/InlineCost.cpp | ||
---|---|---|
211 ↗ | (On Diff #88925) | I'd keep this above with the general implementation routines rather than with the visitors. Also, I'd keep the comment on the implementation rather than the declaration. |
216 ↗ | (On Diff #88925) | Using a function_ref seems like a fairly heavy abstraction. Maybe make this a template and take an arbitrary callable? |
465 ↗ | (On Diff #88925) | What if there are more than two operands? Maybe you want to require one of th eexpression subclasses here? Or maybe just make COps be a small vector so this is generically correct? It should't be any less efficient, and I find the N++ bit below somewhat easy to miss anyways. |
473–476 ↗ | (On Diff #88925) | I would invert and use early return here as well: auto *C = Evaluate(COpts); if (!C) return false; SimplifiedValues[&I] = C; return true; |
507–509 ↗ | (On Diff #88925) | For all of these where you only use the lambda once, put the lambda in the arguments? Also for all of these, can you let the return type be deduced? I always prefer that when it is unambiguous. This might be made easier by the routine accepting a generic callable. |
Thanks for the comments. Will update the revised patch in a bit.
lib/Analysis/InlineCost.cpp | ||
---|---|---|
465 ↗ | (On Diff #88925) | Changed it to use SmallVector |
507–509 ↗ | (On Diff #88925) | Made the suggested change in all but one place where the body of the law is multiple lines and to me makes the surrounding if more readable. |