In D57779, We found that currently that SLP allows assembling bundles with the same operation but different IR flags, for example: "add nsw + add nuw nsw", which is not correct. This change fixes the issue. Also, we might want to use an alternative operation for such operations to unite, I plan to implement this ability in the following change.
Details
Diff Detail
Event Timeline
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 ?
lib/Transforms/Vectorize/SLPVectorizer.cpp | ||
---|---|---|
345 | Maybe use enum class instead. | |
362 | 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; | |
376 | Move the declarations just before if (IsBinOp). BinOpFlags OpFlags = getBinOpFlags(VL[BaseIndex]); | |
394 | Following the naming convention, InstOpFlags should be VL[Cnt]'s flags: 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: What do you think? |
Please can you add a better explanation of the problem to the description of the patch?
I'm not sure what the problem is, you are allowed to drop nuw/nsw flags: https://rise4fun.com/Alive/plNm
So the new vectorized binop should simply take the smallest common subset of flags, which likely most often means no flags.
@dtemirbulatov It might be worth adding tests that check that we do retain the common subset?
Maybe use enum class instead.