[PATCH] D79100: [LV] Emit new IR intrinsic llvm.get.active.mask for tail-folded loops

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM with the comment, thanks! Please wait a day or so with committing in case there are additional comments. Also, I might have missed it, but it would be good to have a test case with unroll-factor/interleave-count > 1 (as mentioned in an inline comment).



================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:1395
+  if (TailFolded)
+    return true;
   return false;
----------------
SjoerdMeijer wrote:
> fhahn wrote:
> > Just return `TailFolded`? 
> > 
> > side note: what are all the extra parameters needed/used somewhere?
> Just a quick first comment on your side note:
> 
> > what are all the extra parameters needed/used somewhere?
> 
> That's just to provide an interface consistent with most other TTI functions here. Currently, we only look at `TailFolded`, the rest is indeed unused. But if (other) targets want to do a bit more analysis here, they most definitely want to look  look at `L` and `LI`, and possibly `SE`.
> 
> So, as I said, mostly unused at the moment, and added for consistency, which I accept may or may not be a good reason. I have no strong opionions, so will refactor this TTI hook right away if you think that is better/necessary.
> 
> I will address your other remarks soon. Thanks for that.
> 
> 
I am not sure, but I think adding all those unused parameters, because they might get used in the future seems a bit odd. I would be easy enough to add them if required.


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

https://reviews.llvm.org/D79100