[PATCH] D62938: [SLP] Forbid to vectorize bundles with same opcode but different IR flags

[Vasileios Porpodas via Phabricator via llvm-commits]

vporpo added a comment.

I am not sure about the value safety assumptions here. Is there any unsafe-math flag that would allow us to override the integer overflow flags?

@dtemirbulatov  Could you also add a couple of lit tests to check whether the patch works as expected ?



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:345
 
+enum BinOpFlags {NoFlag, NUW, NSW, NUWNSW};
+
----------------
Maybe use enum class instead.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:362
+  BinOpFlags Ret = NoFlag;
+  if (!isa<OverflowingBinaryOperator>(I))
+    return Ret;
----------------
I think the whole code could be simplified to something like this (or the equivalent with if/elseif and early returns):
```
bool hasNUW = I->hasNoUnsignedWrap();
bool hasNSW = I->hasNoSignedWrap();
return (hasNUW && hasNSW) ? NUWNSW : hasNUW ? NUW : hasNSW ? NSW : NoFlag;
```



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:376
                                        unsigned BaseIndex = 0) {
+  BinOpFlags MainOpFlags, AltOpFlags;
+
----------------
Move the declarations just before if (IsBinOp).
Also initialize them, and try to use the variable name convention used by the surrounding code. I think you can follow the same structure as the existing code by simply extending the opcode checks to include the flags checks.

BinOpFlags OpFlags = getBinOpFlags(VL[BaseIndex]);
BinOpFlags AltOpFlags = OpFlags;


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:394
   for (int Cnt = 0, E = VL.size(); Cnt < E; Cnt++) {
     unsigned InstOpcode = cast<Instruction>(VL[Cnt])->getOpcode();
+    BinaryOperator *BOP = dyn_cast<BinaryOperator>(VL[Cnt]);
----------------
Following the naming convention, InstOpFlags should be VL[Cnt]'s flags:
BinOpFlags InstOpFlags = getBinOpFlags(VL[Cnt]);

I think you could (and probably should) reuse the original structure of this code since the flag checks are just supplementing the opcode checks. For example, whenever there is a check (InstOpcode == Opcode), it should now be (InstOpcode == Opcode && InstOpFlags == OpFlags)

Similarly the if condition of line 377 should now be:
if ((InstOpcode == Opcode && InstOpFlags == OpFlags) || (InstOpcode == AltOpcode && InstOpFlags == AltOpFlags))

What do you think?




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

https://reviews.llvm.org/D62938