[PATCH] D76965: [FunctionAttrs][Mem2Reg] Handle Alloca passed as function call operand with function attributes

[Dominic Chen via Phabricator via llvm-commits]

ddcc marked 3 inline comments as done.
ddcc added a comment.

In D76965#1950042 <https://reviews.llvm.org/D76965#1950042>, @jdoerfert wrote:

> I'm unsure about the scope of this. It seems to match a particular pattern and it is unclear this is the right place to do so. Have you considered doing this as part of `AAPrivatizablePtr` (or a new AbstractAttribute) in the Attributor?


No, I'm not familiar with the attributor, but I thought that was the component that infers function attributes? I admit I'm a bit hesitant to revise and reimplement this in a different framework. The original reason I ended up implementing this here is that in some benchmark code, I noticed the optimization sequence of global variable localization and memory to register conversion was being inhibited by these attributed functions, when compared against a baseline without attributed functions.



================
Comment at: llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp:317
   // Knowing that this alloca is promotable, we know that it's safe to kill all
   // instructions except for load and store.
-
----------------
jdoerfert wrote:
> This comment will be outdated.
I'll elaborate more here, but this function must still remove all non load/store instructions using this alloca, because the rest of the memtoreg pass assumes it. The only change is that a promotable alloca passed as a non-capturing and non-read function call argument is changed to use a separate fresh alloca, instead of the original one.


================
Comment at: llvm/test/Transforms/Mem2Reg/fnattr.ll:1
+; RUN: opt -S < %s -mem2reg | FileCheck %s
+
----------------
jdoerfert wrote:
> If it is OK with you, please run `update_test_checks.py --function-signature --scrub-attributes` on this file to create the check lines. I personally find it way easier to read as almost complete check lines.
Ok, done.


================
Comment at: llvm/test/Transforms/Mem2Reg/fnattr.ll:111
+    ret i32 %d
+}
----------------
jdoerfert wrote:
> We should do something with the return value of `is_same`, either here or in `is_same`. Dead code for testing is not always future proof. Similarly in other test cases.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76965