[PATCH] D38392: Disallow sinking of unordered atomic loads into loops

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:582
+  // loop body.
+  const bool SinkingToLoopBody = !SafetyInfo;
   // Loads have extra constraints we have to verify before we can hoist them.
----------------
reames wrote:
> Deriving this from whether an optional parameter is present is very fragile.  Please just add another parameter.  
> 
> Reads further: Oh, you're not introducing this, just hoisting it in the function.  Mind doing a separate NFC patch to expose the parameter?
> 
> This could be either before this patch (no further review needed) or after, I don't care.
Here is declaration of canSinkOrHoistInst (LoopUtils.h):
```
/// Returns true if the hoister and sinker can handle this instruction.
/// If SafetyInfo is null, we are checking for sinking instructions from
/// preheader to loop body (no speculation).
/// If SafetyInfo is not null, we are checking for hoisting/sinking
/// instructions from loop body to preheader/exit. Check if the instruction
/// can execute speculatively.
/// If \p ORE is set use it to emit optimization remarks.
bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
                        Loop *CurLoop, AliasSetTracker *CurAST,
                        LoopSafetyInfo *SafetyInfo,
                        OptimizationRemarkEmitter *ORE = nullptr);
```

As you can see it is safe to use SafetyInfo for such check. Actually this line is the only use of SafetyInfo in the function, so I am going to replace this parameter with bool in a separate NFC after this patch.


https://reviews.llvm.org/D38392