It's not legal inventing calls to operator delete.
Side note: instcombine speculates these calls to free using pattern matching.
In my opinion, it shouldn't. Hoisting shouldn't be done in instcombine, and hoisting shouldn't be done on a per-name basis. Presumably, the frontend should attach an attribute to this call to make sure they can be hoisted.
If there's not enough information in the frontend, then the middle-end should compute this information (the attribute) and then some pass should be responsible for the sinking/hoisting.
This is a really confusing name. It sounds like it's detecting whether a call is free of side-effects, whereas it's *actually* detecting whether the call is to a free-like function that has side-effects.
I really don't like duplicating this list here, in a way that means we would miscompile again if the list in isFreeCall is extended but this list is not. (Notably, this list is already missing cases from isFreeCall, and as it happens, the one in isFreeCall is also missing cases.)
Perhaps instead changing this function to determine whether a call is known to be side-effect free when given a null pointer, and special-casing the very small number of functions for which that's true, would be a better and more maintainable approach?
Note that this function returns the wrong value for a nobuiltin call, and thus may still miscompile calls to free when building with -fno-builtin=free.
This is wrong too. Either passing undef to *any* pointer parameter in a function call results in UB, or you cannot assume that passing undef to ::operator delete results in UB. The user-provided ::operator delete might ignore its argument, for example.
I don't know if this is the reason, but LLVM miscompiles calls to operator delete in cases where it ignores its argument too: https://godbolt.org/g/dvvirQ
Wrong for operator delete.
So, from what I can see no transformation made for free() is valid in case of operator delete.
What do you think instead of introducing a predicate isCallToOperatorDelete() and bail out at the beginning of visitFree() ?
Alternatively, we could add a boolean argument to isFreeCall() which doesn't include operator delete (still trying to pick a name for the argument).
I'm leaning towards 1), what do you think?
Not really happy about the name either, have a better proposal?
Sounds fine (but we should also check whether the call is a nobuiltin call to free, which should also not be optimized).
Maybe freeCallHasSideEffects? (Or, if we revert the sense of this as suggested below, freeCallHasNoSideEffects.)
This should presumably also be checking whether the call is nobuiltin.