[PATCH] D69542: Full Restrict Support - single patch

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere marked an inline comment as not done.
jeroen.dobbelaere 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
> 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 ?



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

https://reviews.llvm.org/D69542