[PATCH] D44415: [PM][FunctionAttrs] add NoUnwind attribute inference to PostOrderFunctionAttrs pass

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

Hi, thanks for the patch.

I like the overall approach of making this pass generic.  But I have some high-level comments on the patch:

- Is it possible to explain in the code itself everything a user needs to know to understand the code?  For example, I could not find where we define "derefinable", or derefinement.  More broadly, it is not easy for me to follow the overall structure of this code just by reading the code.

- It's a nit, but I would prefer to write all comments in proper English, so starting with capital letters, ending with periods (if they are complete sentences), using apostrophes in contractions (e.g. cant -> can't), etc.  I think this will make it easier for others to read.

- I'm somewhat confused by our notion of some instructions being "invalid".  Perhaps that should have a different name.

- I'm a bit concerned about the overhead of calling multiple virtual functions (std::function) for every instruction in the module, especially since our loop structure (which is the correct one!) does not have one easily-predicted virtual function call target, but instead iterates between call targets.  I know we go through a lot of pain elsewhere in LLVM to avoid this.   Since the list of functors is static, I wonder if we shouldn't metaprogram this.  Using llvm::integer_sequence would make it not *too* bad.

  But maybe that's a premature optimization; I definitely don't want to do it if there's no benefit.  Perhaps @chandlerc can comment on this.


Repository:
  rL LLVM

https://reviews.llvm.org/D44415