[PATCH] D91690: [LoopFlatten] Widen IV, cont'd

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:551
 
-  auto HasSExtUser = [] (Value *V) -> Value * {
+  auto GetTruncValue = [] (Value *V) -> Value * {
     for (User *U : V->users() )
----------------
dmgreen wrote:
> Is is better to introduce a new trunc of FI.OuterInductionPHI to the correct bitwidth? I'm a little worried that this is just finding _some_ trunc, not necessarily one that it should. It may introduce more truncs but they should get cleared up. It would also prevent using something that did not dominate.
> 
> Maybe it is fine like this, if it is know that the widening will have introduced a trunc. Can it at least check the type of the trunc is correct? And add a comment saying it should have been added by widening.
Since we promote the IV there has to be a trunc back to its users. What I see is that there is 1 trunc instruction, and then different zexts instructions of the IV value that to different users if they have different types. This means there is 1 trunc instruction, but you're right that this is not the whole story and something is missing at the moment.  So, we will need a generic way to map the different values with the different users, if there are any. I think, for now, I will add a check a bit earlier in the pipeline to bail if we find more than 1 zext user of that trunc. That won't be optimal, but I am keen to start somewhere with this. And of course this patch in its current shape is still running in an assert when I just tried it out with different trunc users slightly modifying your example case:

  void test(char n, char m) {
  for(char i = 0; i < n; i++)
    for(char j = 0; j < m; j++) {
      char x = i*m+j;
      use_32(x);
      use_16(x);
    }
  }


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

https://reviews.llvm.org/D91690