llvm dev - Nov 2016 - [SCEV] inconsistent operand ordering

Thanks for the helpful reply!

I see that we are trying to keep ScalarEvolution stable around instruction
ordering. My suggestion would be to not restrict the fix by only recursing on
the first operand.

By "dominator logic"  I meant that if all other 'cheap' checks
fail, we should decide by walking the dominator tree to see which
instruction's basic block is encountered first from the function entry
block. But we obviously cannot use this if we want ScalarEvolution to be stable
around 'simple' CFG changes.

Thanks,
Pankaj


-----Original Message-----
From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com] 
Sent: Tuesday, October 18, 2016 12:01 PM
To: Chawla, Pankaj
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [SCEV] inconsistent operand ordering

Hi Pankaj,

Chawla, Pankaj wrote:
> Thanks for fixing this Sanjoy!
>
> I do have a few questions/suggestions on the fix if you don't mind.
>
> 1) Would this work correctly if the values are call instructions with 
> no operands, like this- %a = foo() %b = bar()
>
> 2) From the way this function is set up, it looks like the emphasis is on
saving compile time by trading off robustness. Is compile time such a big
concern here that we want to fix this one test case at a time? We can perhaps
use dominator logic to fix this once and for all.
For (1), in addition to the recursion, I think we'll need to differentiate
between global variables.

For (2), I'm not quite sure what you mean by "dominator logic".

I agree with you generally: within reason, we should try to canonicalize as much
as possible.
> 3) When both instructions are in the same basic block, iterating the bblock
and returning based on the instruction which is encountered first might be
better.
This was my first impulse, but I'll have to think about whether:

  a. It makes SCEV too fragile around instruction ordering -- we don't want
divergent behavior triggered by seemingly innocent ordering differences.

  b. What obligations will then exist for passes that move instructions with a
basic block, since that can now change the definition of canonical SCEV
expressions.

-- Sanjoy

Hi Pankaj,

On 10/18/16, 1:25 PM, Chawla, Pankaj wrote:
 > I see that we are trying to keep ScalarEvolution stable around
 > instruction ordering. My suggestion would be to not restrict the fix
 > by only recursing on the first operand.

I think I've addressed this in https://reviews.llvm.org/rL285535, PTAL.

-- Sanjoy

LGTM.
Thanks for fixing this!

-Pankaj

-----Original Message-----
From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com] 
Sent: Sunday, October 30, 2016 9:02 PM
To: Chawla, Pankaj <pankaj.chawla at intel.com>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [SCEV] inconsistent operand ordering

Hi Pankaj,

On 10/18/16, 1:25 PM, Chawla, Pankaj wrote:
 > I see that we are trying to keep ScalarEvolution stable around  >
instruction ordering. My suggestion would be to not restrict the fix  > by
only recursing on the first operand.

I think I've addressed this in https://reviews.llvm.org/rL285535, PTAL.

-- Sanjoy