Page MenuHomePhabricator

[FunctionAttrs][Mem2Reg] Handle Alloca passed as function call operand with function attributes
Needs ReviewPublic

Authored by ddcc on Mar 27 2020, 6:46 PM.

Details

Reviewers
jdoerfert
Summary

Allow mem2reg to replace an Alloca used as a nocapture and inaccessiblememonly/readnone function call operand with undef

Diff Detail

Unit TestsFailed

TimeTest
20 msMLIR.Dialect/Affine::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/Dialect/Affine/loop-permute.mlir -test-loop-permutation="permutation-map=1,2,0" | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/Dialect/Affine/loop-permute.mlir --check-prefix=CHECK-120

Event Timeline

ddcc created this revision.Mar 27 2020, 6:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 6:46 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
jdoerfert added a comment.EditedMar 28 2020, 12:00 AM

Allow mem2reg to replace an Alloca used as a nocapture and inaccessiblememonly/readnone function call operand with undef

This is not sound. Take the following example

int foo(int *a) {
  return a & 31;  // or "worse": if(a&32) return 1; else return 0;
}
int is_stack_aligned() {
   int stack;
   return foo(&stack);
}

Replacing stack with undef will introduce poison or UB where there was none before.
It also breaks if you have something like:

int is_same(int *a, short *b) {
  return a == b;
}
int return_true() {
  int Stack;
  return is_same(&Stack, (short*)&Stack);
}

That said, I think what we can do is "privatize" the pointer under more strict requirements than you have here. I was hoping to do that in AAPrivatizablePtrImpl at some point but I now think the requirements we have there are still a little too weak. I'll have to think about this one a bit. (Btw. you should really consider implementing some of these in the Attributor instead. It should be way more powerful there.)

jdoerfert requested changes to this revision.Mar 28 2020, 12:01 AM
This revision now requires changes to proceed.Mar 28 2020, 12:01 AM
ddcc added a comment.Mar 28 2020, 3:37 PM

Hmm, yeah, undef is probably too strong. Ok, how about I replace the argument with a fresh alloca? It should still permit load/store optimizations on the original alloca, while still providing some alloca that isn't accessed.

I'm not familiar with the attributor, is that the component that infers function attributes? Isn't that somewhat orthogonal since it tags function attributes for subsequent optimizations like this one?

ddcc updated this revision to Diff 253380.Mar 28 2020, 3:37 PM

Create fresh alloca instead of undefvalue

I'll check later but please add my examples as tests.

ddcc updated this revision to Diff 253397.Mar 28 2020, 10:14 PM

Add more tests, fix bugs

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?

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
317

This comment will be outdated.

llvm/test/Transforms/Mem2Reg/fnattr.ll
2

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.

112

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.

ddcc marked 3 inline comments as done.Mar 30 2020, 12:53 PM

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.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
317

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.

llvm/test/Transforms/Mem2Reg/fnattr.ll
2

Ok, done.

112

Done.

ddcc updated this revision to Diff 253672.Mar 30 2020, 12:54 PM

Update tests with update_test_checks.py, fix bug