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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
