[PATCH] D97677: [WebAssembly] Fix more ExceptionInfo grouping bugs

[Thomas Lively via Phabricator via llvm-commits]

tlively accepted this revision.
tlively added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExceptionInfo.cpp:207
 
-  // Add BBs to exceptions
+  // Add BBs to exceptions' block vector
   for (auto DomNode : post_order(&MDT)) {
----------------
aheejin wrote:
> tlively wrote:
> > I expected to see code copying the contents of the block set to the block vector, but I don't see that. Do the set and the vector have different contents, or am I missing something else?
> We can do it that way. We are just using a map from BB to exception (look at `getExceptionFor` below, in line 210) to do that now.
> 
> So previously the code was
> ```
>   for (auto DomNode : post_order(&MDT)) {
>     MachineBasicBlock *MBB = DomNode->getBlock();
>     WebAssemblyException *WE = getExceptionFor(MBB);
>     for (; WE; WE = WE->getParentException())
>       WE->addBlock(MBB);
>   }
> ```
> 
> And `addBlock` was doing this:
> ```
>     Blocks.push_back(MBB);
>     BlockSet.insert(MBB);
> ```
> 
> Now we are doing the same thing in two steps.
> ```
>   for (auto DomNode : post_order(&MDT)) {
>     MachineBasicBlock *MBB = DomNode->getBlock();
>     WebAssemblyException *WE = getExceptionFor(MBB);
>     for (; WE; WE = WE->getParentException())
>       WE->addToBlockSet(MBB);
>   }
> 
>   ...
>   ... Fix incorrect mappings ...
>   ...
> 
>   for (auto DomNode : post_order(&MDT)) {
>     MachineBasicBlock *MBB = DomNode->getBlock();
>     WebAssemblyException *WE = getExceptionFor(MBB);
>     for (; WE; WE = WE->getParentException())
>       WE->addToBlockVector(MBB);
>   }
> ```
I see, thanks. Either way is fine with me.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExceptionInfo.h:48
   std::vector<MachineBasicBlock *> Blocks;
-  SmallPtrSet<const MachineBasicBlock *, 8> BlockSet;
+  SmallPtrSet<MachineBasicBlock *, 8> BlockSet;
 
----------------
aheejin wrote:
> tlively wrote:
> > If we expect there to be few blocks, then perhaps the cost of deleting them from the vector wouldn't be too bad? Alternatively, we could also "remove" blocks by replacing them in the vector with nullptr, then compact the vector and remove the nullptrs just once at the end. That might be more complicated then just adding this set, though.
> The number of blocks varies greatly. There are many exceptions with fewer than 8 BBs so I set it to 8, but it does not mean I think that will be the median. There are functions dozens of BBs are within the `catch` part, and removing a block from those vectors will be expensive. Also when we remove a BB from, say, Exception A, it can involve multiple removal, because the BB's innermost containing exception might not be Exception A. If Exception A > B > C and BB belongs to C and we want to remove BB from Exception A, we have to remove BB form all A, B, and C's vectors.
> 
> Yes, I think removing things by setting them to nullptr and compressing the vector later is OK too. I just think the set is more convenient for now.. `MachineLoop` also keeps both a set and vector, even though it doesn't mean we should do the same. (I couldn't remove the vector because `SortRegion` was expecting some unified interface on it, but I think we can remove the set without affecting `SortRegion`, but yeah, it's just convenient to keep it..)
> 
> I also thought about removing the vector and using sets only within `SortRegion` but it was tricky due to some constness difference; `MachineLoop`'s vector contains `MachineBasicBlock*` but its set contains `const MachineBasicBlock*`...
I agree that using a set seems reasonable, especially given the precedent in MachineBasicBlock. Thanks for the info!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97677