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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev marked 2 inline comments as done.
fedor.sergeev added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1082
+    /// scan
+    bool SuccessfullyScanned = false;
+
----------------
fedor.sergeev wrote:
> jlebar wrote:
> > fedor.sergeev wrote:
> > > jlebar wrote:
> > > > I don't know how Chandler feels about it, but personally I'm not wild about the approach of sticking state into this struct.  It uses the descriptor class for two quite different purposes, and perhaps worse, it's really easy to have a bug where we forget to reset ScanCurrent at the top of the loop...
> > > > 
> > > > I *think* Chandler's suggestion was, instead of tracking the state with a bitset, track it by removing elements from a list (or adding them to a list of "attributes to use when scanning this function").
> > > > 
> > > > It looks like the difficulty you face here is that the list you want to remove elements from is also the list you're iterating over?  I'd be OK with pretty much any of the obvious solutions to that -- remove and adjust the loop index, use an std::list and remove in place, remove after the loop completes...
> > > > 
> > > > Dunno if Chandler has feels about this too.
> > > Well, I dont like merging descriptor with state either.
> > > In my original implementation I had those in separate vectors, which forced me to use indices as the way to link descriptors and states.
> > > 
> > > Chandler's suggestion indeed was to remove elements.
> > > However it does not play well with the need to have as many early exits as possible (which was a property of implementation before my changes).
> > > Namely, there is a single meaningful vector here, a vector of descriptors, and while I can use removal from it to signify a single binary fact (e.g. Valid), there are more facts to track. Should I introduce copies of the main vector? How would it be different from having bit-vectors?
> > > 
> > > I really do not see an ideal solution here.
> > > 
> > > I can introduce a vector< pair<descriptor,state> >, which is rather clumsy.
> > > Or I can introduce a more complicated interface to the descriptor class, with read/write accessors as needed,
> > > and service functions that do initialization of mutables as needed.
> > > Should I introduce copies of the main vector?
> > 
> > That's what I was thinking.
> > 
> > > How would it be different from having bit-vectors?
> > 
> > I think the notion was that you'd iterate over the vector that's being modified, so there's less indirection in the code.
> > 
> > In pseudocode:
> > 
> > ```
> > ScannedSomeFunction = false;
> > 
> > SCCDescriptors = InterfaceDescriptors;
> > for (F : SCCNodes) {
> >   FnDescriptors = []
> >   for (Desc : SCCDescriptors) {
> >     if (F->isDeclaration() || (ID.RequiresExactDefinition && !F->hasExactDefinition())) {
> >       SCCDescriptors.Remove(Desc);
> >     } else if (!Desc.SkipFunction(F)) {
> >       FnDescriptors.Append(F);
> >     }
> >   }
> > 
> >   for (I : instructions(F)) {
> >     if (FnDescriptors.empty())
> >       break;
> > 
> >     for (Desc : FnDescriptors) {
> >       if (Desc.InstrBreaksAttribute(I)) {
> >         FnDescriptors.Remove(Desc);
> > 	SCCDescriptors.Remove(Desc);
> >       }
> >     }
> >   }
> > 
> >   for (Desc : SCCDescriptors) {
> >     if (Desc.SkipFunction(F))
> >       continue;
> >     Desc.SetAttribute(F)
> >   }
> > }
> > ```
> > 
> > I don't have SuccessfullyScanned here, but I *think* it's actually not necessary, because SkipFunction is the only reason we wouldn't scan something, and if we skipped it, we also won't set the attr on it.
> Ahem... that should work, indeed. Lemme try it.
Done more or less along the lines of your suggestion, Justin (applying Chandler's advice about using erase_if).
The only catch is that I had to add AttrKind to the descriptor to be able to match descriptors in different vectors
(to implement your : FnDescriptors.Remove + SCCDescriptors.Remove).



Repository:
  rL LLVM

https://reviews.llvm.org/D44415