[PATCH] D34188: Don't check side effects for functions outside of SCoP

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a subscriber: sameer.abuasal.
grosser added a comment.

Hi Sanjay,

thanks for your feedback. This is very valuable!

@sameer.abuasal: Our discussion does not seem to hold back your patch, but as your patch triggered it, it seems OK to complete it here. Feel free to commit.

@singam-sanjay: I replied to you inline. Please keep asking if my answers need further clarification.

Best,
Tobias



================
Comment at: lib/Support/SCEVValidator.cpp:121
 bool polly::isConstCall(llvm::CallInst *Call) {
   if (Call->mayReadOrWriteMemory())
     return false;
----------------
singam-sanjay wrote:
> For polyhedral analysis, do we also need the function to evaluate to the same value always (not just side effect free) inside the Scop ? I got this notion from the name of the function.
> 
> If that's the case, is it possible to check for that ? ( I have a feeling that it's not feasible )
Yes, that's what we need. I believe that we can assume that if a function does not read memory it will always return the same value.

However, there is another bug that slipped in. I assumed mayReadOrWriteMemory corresponds to is-always-executed. This is not the case, so we should likely limit this to functions that are called in the entry block (or at least post-dominate the entry block).


================
Comment at: lib/Support/SCEVValidator.cpp:125
   for (auto &Operand : Call->arg_operands())
     if (!isa<ConstantInt>(&Operand))
       return false;
----------------
singam-sanjay wrote:
> Can we consider `llvm::ConstantFP` as well ? Or any constant type ?
We use this for ScalarEvolution expressions, which are all integral. So I am note really seeing if/when we would need other constant types. In general, I made good experience with working use-case driven. Meaning, in case we have a benchmark/code that could benefit from such constructs, we add them.


Repository:
  rL LLVM

https://reviews.llvm.org/D34188