[PATCH] Using Masked Load / Store intrinsics in Loop Vectorizer

[Demikhovsky, Elena]

-  Elena


-----Original Message-----
From: Michael Zolotukhin [mailto:mzolotukhin at apple.com] 
Sent: Tuesday, December 09, 2014 05:48
To: Demikhovsky, Elena; nrotem at apple.com; aschwaighofer at apple.com; mzolotukhin at apple.com
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] Using Masked Load / Store intrinsics in Loop Vectorizer

Hi Elena,

Please see my comments inline.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5386-5387
@@ +5385,4 @@
+          !SI->getParent()->getSinglePredecessor()) {
+        if (canPredicateStore(SI->getValueOperand()->getType(),
+                                  SI->getPointerOperand())) {
+          MaskedOp.insert(SI);
----------------
delena wrote:
> mzolotukhin wrote:
> > Could this condition be simplified? E.g. we can store result of canPredicateStore(..) in a bool variable and use it here.
> > Also, I think it might be incorrect - shouldn't we insert SI to MaskedOp even if NumPredStores hasn't exceeded NumberOfStoresToPredicate?
>  If  NumPredStores hasn't exceeded NumberOfStoresToPredicate I'll not generate  masked store. I want to keep the current behavior.
> 
> Something like this?
>       bool isSafePtr = (SafePtrs.count(SI->getPointerOperand()) !=0);
>       bool isSinglePredecessor = SI->getParent()->getSinglePredecessor();
>       
>       if (++NumPredStores > NumberOfStoresToPredicate || !isSafePtr ||
>           !isSinglePredecessor) {
>         // Build a masked store if it is legal for the target, otherwise scalarize
>         // the block.
>         bool isLegalMaskedOp = canPredicateStore(SI->getValueOperand()->getType(),
>                                                  SI->getPointerOperand());
>         if (isLegalMaskedOp) {
>           --NumPredStores;
>           MaskedOp.insert(SI);
>           continue;
>         }
>         return false;
>       }
> 
Actually, I wanted to get rid of ++NumPredStores, followed by --NumPredStores.
I'd rewrite it like this:

```
if (!isSafePtr || !isSinglePredecessor) {
    if (isLegalMaskedOp) {
        // This store will be masked, no predication is needed for it.
        MaskedOp.insert(SI);
        continue;
    } else {
        // This store needs to be predicated.
        if (++NumPredStores > NumberOfStoresToPredicate)
            return false;
    }
}
```
Also, I wonder if we actually need a 'continue' statement in if(isLegalMaskedOp) - do you intentionally skip subsequent checks (e.g. it->mayThrow) for this case?
[Demikhovsky, Elena] Some tests fail in this case, I tried. Because NumberOfStoresToPredicate allows you to predicated some stores without masking.
I wanted to keep this knob as is.
I also do not check mayThrow() because it is for calls only.

http://reviews.llvm.org/D6527


---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.