[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:
> 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.
> I believe I've seen similar behavior of the SimplifyCFG pass, I'll try to prepare a test for that, too.

It appeared to be the same pass (LoopRotate). The dump just did not show the change with -print-after-all.


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

https://reviews.llvm.org/D69542