[PATCH] D69542: Full Restrict Support - single patch

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopRotationUtils.cpp:494
       // Otherwise, create a duplicate of the instruction.
       Instruction *C = Inst->clone();
 
----------------
barannikov88 wrote:
> jeroen.dobbelaere wrote:
> > barannikov88 wrote:
> > > @jeroen.dobbelaere
> > > In case the instruction is Load or Store, its provenance operand is set UndefValue upon cloning. Is this behavior appropriate here?
> > @barannikov88 
> > Good question. The restrict patches have had already a number of iterations. In the earlier versions, ptr_provenance information was dropped when cloning. The main reason was for safety: In a restrict world, you need to think what to do when cloning an instruction wrt the restrict validity: do we need to keep the same scopes/restrict variable or do we need to work on a copy. Dropping the info was a safety guard. But, we needed to keep the same number of arguments, so I used `UndefValue` to indicate a dropped ptr_provenance.
> > 
> > At that time, a missing ptr_provenance would result in 'safe analysis', as, both a ptr_provenance and a !noalias metadata were required to perform ScopeNoAliasAA. Later on, it became clear that we should also do analysis if no ptr_provenance was explicitly present (which meant the same as ptr == ptr_provenance) and the rules changed.
> > 
> > This probably means that just dropping the ptr_provenance when cloning is probably not the right thing to do any more.
> > And passes that use `clone()` on a load/store instruction should become aware of the potential extra dependency introduced by the ptr_provenance. Dropping it (in a restrict) world should only be done if also the !noalias metadata is removed. In a world where ptr_provenance is also used for tracking actual ptr_provenance, this might also not be true.
> > 
> > So no, this behavior is probably not appropriate here. Do you happen to have a testcase that confirms this ?
> > 
> Thank you for the quick and detailed answer!
> I have a testcase, but right now it contains some bits of proprietary information. I'll strip them off and post the test here later.
> 
Here you go:
{F14761641}

`$ opt test.ll -loop-rotate -S -o -`

You should be able to find the following line in the output:
`%x.01 = load i8, i8* %src_ptr, ptr_provenance i8* undef, align 1, !tbaa !10, !noalias !9`

The patch was aplied to the same base, the project is configured as:
```
-DBUILD_SHARED_LIBS=ON
-DLLVM_CCACHE_BUILD=ON
-DLLVM_TARGETS_TO_BUILD="X86"
-DLLVM_ENABLE_PROJECTS="clang"
-DLLVM_USE_LINKER=lld
```

PS
I believe I've seen similar behavior of the SimplifyCFG pass, I'll try to prepare a test for that, too.


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

https://reviews.llvm.org/D69542