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

[Singapuram Sanjay Srivallabh via Phabricator via llvm-commits]

singam-sanjay added inline comments.


================
Comment at: lib/Support/SCEVValidator.cpp:121
 bool polly::isConstCall(llvm::CallInst *Call) {
   if (Call->mayReadOrWriteMemory())
     return false;
----------------
grosser wrote:
> 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).
> 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.

What about random number generators ?

> 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).

How does mayReadOrWrite become mustReadOrWrite for those functions ?





================
Comment at: lib/Support/SCEVValidator.cpp:125
   for (auto &Operand : Call->arg_operands())
     if (!isa<ConstantInt>(&Operand))
       return false;
----------------
grosser wrote:
> 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.
> We use this for ScalarEvolution expressions, which are all integral.

I had this doubt in the context of detecting **any** ConstCall to a function, including something like `sqrt(2.3) `.

Thanks for the info ! it was very helpful.

> Meaning, in case we have a benchmark/code that could benefit from such constructs, we add them.

Makes sense.


================
Comment at: lib/Support/SCEVValidator.cpp:396
 
   ValidatorResult visitUnknown(const SCEVUnknown *Expr) {
     Value *V = Expr->getValue();
----------------
grosser wrote:
> singam-sanjay wrote:
> > How is this function called ? I don't find any explicit call to this function and the code [[ http://llvm.org/reports/coverage/tools/polly/lib/Support/SCEVValidator.cpp.gcov.html | coverage reports ]] are a month old.
> I wrote an email to @sylvestre.ledru (see llvm-dev) to check about the status of the coverage reports.
> 
> It is called by the SCEVVisitor which serves as base class for the SCEVValidator.
> I wrote an email to @sylvestre.ledru (see llvm-dev) to check about the status of the coverage reports.

I thought the coverage reports were automated, like CI services, which start compiling as soon as a commit is made.

> It is called by the SCEVVisitor which serves as base class for the SCEVValidator.

Oh ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D34188