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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1080
+
+    /// true for "valid" attributes that went through at least one instruction
+    /// scan
----------------
fedor.sergeev wrote:
> jlebar wrote:
> > Capital
> I wonder if capitalizing "true" which is a boolean value and not an English word here is fine?
I think of the boolean as *being* true, which in C++ might mean it has value `true`, in Python might mean it has value `True`, in PHP might mean it has value `"True"` :)...  That is, I think you can say "this Python variable is true" with a lower-case "t", and that's just fine.

But that's quite a nit, whatever you like is fine.  :)


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1082
+    /// scan
+    bool SuccessfullyScanned = false;
+
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D44415