wsmoses / Tapir-LLVM

Tapir extension to LLVM for optimizing Parallel Programs
Other
132 stars 24 forks source link

Should I be able to pass nullptr for ExitBlocks into CloneIntoFunction() and CreateHelper()? #14

Closed VictorYing closed 7 years ago

VictorYing commented 7 years ago

I'm in the process of merging all the changes that have been made in Tapir's master since March into my Swarm compiler project, and I notice special handling of exit blocks that might have something to do with C++ exceptions was added in late April. In particular, I notice CloneIntoFunction() and CreateHelper() now have an ExitBlocks argument, which defaults to nullptr in the declaration in include/llvm/Transforms/Tapir/Outline.h. However, CloneIntoFunction() always tries to dereference it at the start of the function, without checking if it is null: https://github.com/wsmoses/Parallel-IR/blob/ce8d1893a0e20842b28ab7561b8e8c6c3e1a051c/lib/Transforms/Tapir/Outline.cpp#L95 so using the default value of nullptr just causes errors. It seems to me that either: 1.) CloneIntoFunction() should check if ExitBlocks is null before dereferencing it, or 2.) The ExitBlocks argument must be non-null, so the default value of nullptr should be removed, perhaps replaced with defaulting to an empty set instead.

I have implemented option 1 in my repo, and would be interested in seeing a similar change in Tapir's master to keep the diff between my repo and Tapir small.

VictorYing commented 7 years ago

Ack, I just noticed that findInputOutputs() suffers from the same problem: https://github.com/wsmoses/Parallel-IR/blob/163a340e81dcf8b55aa55e0d638cc0ce6087541b/include/llvm/Transforms/Tapir/Outline.h#L37-L38 https://github.com/wsmoses/Parallel-IR/blob/163a340e81dcf8b55aa55e0d638cc0ce6087541b/lib/Transforms/Tapir/Outline.cpp#L61