[PATCH] D119792: [Clang] [P2025] Analyze only potential scopes for NRVO

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Sema/Scope.h:518
 
   void addNRVOCandidate(VarDecl *VD) {
+    // every candidate except VD is "spoiled" now, remove them from the set
----------------
Izaron wrote:
> ChuanqiXu wrote:
> > Firstly I am wondering why here doesn't check `NRVO.getInt()` to shortcut directly. But later I found it would change the logic in `::mergeNRVOIntoParent`:
> > ```
> > void Scope::mergeNRVOIntoParent() {
> >   if (VarDecl *Candidate = NRVO.getPointer()) {
> >     if (isDeclScope(Candidate))
> >       Candidate->setNRVOVariable(true);
> >   }
> >   ...
> > ```
> > 
> > It would set NRVO for the candidate in NRVO if it is in current scope. With the context of `addNRVOCandidate` here, I could judge that the change would be:
> > ```
> > X test(bool B) {
> >   X x; // before: no nrvo, after: no nrvo (same)
> >   if (B)
> >     return x;
> >   X y; // before: no nrvo, after: nrvo (better)
> >   return y; // Now NRVO.getInt()==true and NRVO.getPointer() == y;
> > }
> > ```
> > 
> > Yeah, the behavior is still 'right'. `y` should be NRVO in this case. But the implementation smell bad, if `NRVO.getInt()` is true, we shouldn't do any thing. I am not sure if I state my points well. I mean the implementation might be correct, but it is hard to understand, read and maintain. It'd better to make the reader avoid doing mathmatics when reading the codes.
> > I am not sure if I state my points well. I mean the implementation might be correct, but it is hard to understand, read and maintain. It'd better to make the reader avoid doing mathmatics when reading the codes.
> I agree that it is really hard to understand and needs to be polished. It took long time for me to construct the code that won't break.
> 
> I think that one of the sources of the complexity is the `NRVO` variable itself.
> If we could change
> ```
> llvm::PointerIntPair<VarDecl *, 1, bool> NRVO;
> ```
> to something like
> ```
> VarDecl *NRVOCandidate;
> bool InvalidatesParentNRVOCandidates;
> ```
> And maybe rename `setNoNRVO()` to smth like `invalidateNRVOCandidates` and so on.
> 
> What do you think?
>From my understanding, 
```
llvm::PointerIntPair<VarDecl *, 1, bool> NRVO;
```
is structurally equal to
```
VarDecl *NRVOCandidate;
bool InvalidatesParentNRVOCandidates;
```

so it doesn't change anything. My idea is that if it is possible to remove `NRVOCandidatesInScope`? Since there would be at most one NRVO candidate in one scope. So it should be possible to represent the NRVO candidate by one variable if we don't support nested scopes. If it makes sense? Then let me consider the case about nested scopes. For the nested scopes, the child scope is responsible to inform the parent scope that:
     If any variable in the parent scope is an available NRVO candidate.

So the child should offer a set for unavailable NRVO candidates for the parent scope. So we could get the data structure:
```
llvm::SmallMap<VarDecl *, bool> NRVOCandidatesLattices;
```

`NRVOCandidatesLattices[V] == true` indicates that V is known available in current scope and all the child scopes. And `NRVOCandidatesLattices[V] == false` indicates that V is known unavailable in current scope and all the child scopes.

So we could get the implementation for `addNRVOCandidate(VarDecl *VD)`:
```
if (VD not in NRVOCandidatesLattices) {
    NRVOCandidatesLattices[VD] = true; // We met the VD firstly, it should be available NRVO candidate.
    Update NRVOCandidatesLattices for other variables; // There should be only one available variables in NRVOCandidatesLattices.
}
else if (NRVOCandidatesLattices[VD])
    nothing;  // We met the available NRVO again, we shouldn't do anything;
else // NRVOCandidatesLattices[VD] == false;
    nothing;  // We've already know what happened.
```


And in `mergeNRVOIntoParent `, we could do:
```
VarDecl *VD = try to get the only one available NRVO candidate from UnavilableNRVOCandidates;
if (VD && isDeclScope(Candidate))
     set VD as NRVO candidate;

ParentScope->mergeUnavilableNRVOCandidates(UnavilableNRVOCandidates); // We could do lattice merges here. The implementation should be trivial.
```

The implementation looks good to me and easy to understand. Maybe we could add extra data structure to erase the iterations in `mergeNRVOIntoParent ` and the first branch of `addNRVOCandidate `. But they are helpers and wouldn't make the framework hard to understand.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119792/new/

https://reviews.llvm.org/D119792