Fix lifetime call in landingpad blocks simplifycfg from removing the landingpad.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Thanks for the patch!
In the future, please upload with full context (-U99999) as described here:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
| llvm/lib/Transforms/Utils/SimplifyCFG.cpp | ||
|---|---|---|
| 3975 | Can this logic be shared with removeEmptyCleanup? It is the analogous transform for WinEH. That way, if we later add new no-op intrinsics to skip (llvm.assume), both EH styles benefit. | |
| llvm/lib/Transforms/Utils/SimplifyCFG.cpp | ||
|---|---|---|
| 3975 | removeEmptyCleanup has similar logic. Should I adopt the logic from removeEmptyCleanup? | |
| llvm/lib/Transforms/Utils/SimplifyCFG.cpp | ||
|---|---|---|
| 3975 | I put the shared logic into the function isIntrinsics to check if it is one of the specified intrinsics. | |
| llvm/lib/Transforms/Utils/SimplifyCFG.cpp | ||
|---|---|---|
| 3966 | Let's use a more descriptive name. A reader could interpret this to mean, "is this any intrinsic instruction?" I would suggest isCleanupBlockEmpty, and see below for ideas on factoring out the while loop. | |
| 3993 | I think you can factor out this while loop as well, so that the function takes the iterators as a parameter, and then returns a boolean. The caller could look like: if (!isCleanupBlockEmpty(LPInst, RI)) return false; | |
| llvm/lib/Transforms/Utils/SimplifyCFG.cpp | ||
|---|---|---|
| 3993 | The example code you give takes two parameters with instruction type. So, I make the function to take two instructions instead of iterators. | |
Comment Actions
By @lebedev.ri via email:
This likely should ideally also look past new attribute bundles/ephemeral values?
I agree. We need to unify these things instead of matching them explicitly, asking for droppable or ephemeral values. The interface is probably needs to provide some choices, e.g., for debug information, but I think by default treating all intrinsics we can safely remove as droppable should get as almost there.
Comment Actions
Sounds good, but such a classification tool does not exist, and this was Zequan's first patch, so I didn't ask him to create this abstraction.
Building this concept (meta-instructions from codegen seem similar) isn't easy. It's not legal to move code past a lifetime intrinsic, for example. So it's hard to nail down the exact semantics of what we're looking for here.
Comment Actions
Lifetime intrinsics specifically pose their own problem, I think; you can't really just individually drop one. Although, the semantics are sort of fuzzy.
Maybe it would be worth amending LangRef to explicitly allow dropping lifetime.end in general. Not sure what that text would look like. (You wouldn't want to, usually, since it would likely block coloring, but here the payoff might be big enough.)
I have a rough idea for a proposal bouncing around in my head to try to replace lifetime intrinsics with something more powerful/well-defined for local variables, but probably I don't have time to actually implement it anytime soon. Basically, instead of the current lifetime.start/end, we would have intrinsics that look basically like malloc/free, except that the memory allocation is backed by an alloca. In that world, it would be a lot easier to determine what transforms are actually legal/reasonable. If anyone is interested, I could try to write something up more formally.
Can this logic be shared with removeEmptyCleanup? It is the analogous transform for WinEH. That way, if we later add new no-op intrinsics to skip (llvm.assume), both EH styles benefit.