[PATCH] D109917: [InstCombine] Improve TryToSink for side-effecting calls that would be trivially dead
Anna Thomas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 30 08:54:19 PDT 2021
anna added a comment.
In D109917#3027817 <https://reviews.llvm.org/D109917#3027817>, @reames wrote:
> Skimming the code for this and the prerequisite patch, I'm a bit uncomfortable. It really feels like you're trying to a) do several things in one change, and b) possibly abusing an interface for a purpose it wasn't intended.
For others following along: we plan to discuss offline. Will summarize result.
> I would suggest moving the no-alias-call bit to a separate patch as that seems unrelated to the core functionality.
Unfortunately, the no-alias bit is required for the malloc, since the `Scan` instruction *starts* from the instruction we try to sink (i.e. the malloc call) and we fail at `mayWriteToMemory`. We can frame this as possibly a CT win and land it separately.
================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3718
+ (isStackStateMod && Scan->isFunctionStackStateModifying()))
+ return false;
+ }
----------------
reames wrote:
> reames wrote:
> > Hm, I am not sure here, but I think you should check for potential reads here as well.
> >
> > My reasoning is as follows. While the instruction we're moving might be trivially dead if there are no users, it may *not* be trivially dead if there are users. There may be some externally arranged invariant that (say) the result is stored with a volatile variable *if-and-only-if* the call actually has a side effect.
> >
> > As an example, consider the following:
> > a = malloc()
> > stat = malloc_library_counter()
> > if (c)
> > free(a)
> >
> > Basically, is it legal to sink the malloc in this case? I legitimately don't know what the right answer is here.
> >
> > Another example might be:
> > a = malloc()
> > stat = malloc_library_counter()
> > if (c)
> > (volatile i8*)g = a;
> >
> JFYI, my potential cases just described are definitely disallowed by the function comment in the header which describes trivial-deadness. Unfortunately, so is malloc. As such, that comment is definitely wrong, and needs updated. :)
I'm not sure if `trivial-deadness` is the property used in DSE for removing the malloc, but the case above is handled by DSE even if we have an externally arranged invariant through `malloc_library_counter` for example:
cat simple.ll
```
declare noalias i8* @malloc(i32) willreturn
declare i32 @malloc_library_counter()
define void @test20A() {
%m = call i8* @malloc(i32 24)
%stat = call i32 @malloc_library_counter()
store i8 0, i8* %m
ret void
}
` opt -basic-aa -dse -S simple.ll` :
```
define void @test20A() {
%stat = call i32 @malloc_library_counter()
ret void
}
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109917/new/
https://reviews.llvm.org/D109917
More information about the llvm-commits
mailing list