[PATCH] D123720: [VPlan] Replace use of needsVectorIV with VPlan user check.

[Danila Kutenin via Phabricator via llvm-commits]

danlark added a comment.
Herald added a subscriber: pcwang-thead.

Hi @fhahn, this is the root cause for a miscompilation we observed

  #include <optional>
  #include <string>
  #include <string_view>
  #include <iostream>
  
  __attribute__((always_inline)) static inline unsigned char constant_time_ge_8(unsigned int a, unsigned int b) {
    return ~(((unsigned int)(a - b)) >> 15);
  }
  
  __attribute__((noinline)) unsigned char CheckPadding(unsigned char *data, unsigned int length)
  {
      unsigned int padding_length = data[length - 1];
      unsigned char res = 0;
  
      for (unsigned int i = 0; i < length - 1; i++) {
          unsigned char mask = constant_time_ge_8(padding_length, i);
          unsigned char b = data[length - 1 - i];
          res |= mask & (padding_length ^ b);
      }
  
      return res;
  }
  
  __attribute__((noinline)) unsigned char CheckPaddingNoVec(unsigned char *data, unsigned int length)
  {
      unsigned int padding_length = data[length - 1];
      unsigned char res = 0;
  
  #pragma clang loop vectorize(disable)
      for (unsigned int i = 0; i < length - 1; i++) {
          unsigned char mask = constant_time_ge_8(padding_length, i);
          unsigned char b = data[length - 1 - i];
          res |= mask & (padding_length ^ b);
      }
  
      return res;
  }
  
  __attribute__((aligned(16))) unsigned char data_length41[41] = {
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x03, 0x03};
  
  __attribute__((aligned(16))) unsigned char data_length40[40] = {
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x03, 0x03, 0x03};
  
  int main() {
      std::cout << (int)CheckPadding(data_length40, sizeof(data_length40)) << std::endl;
      std::cout << (int)CheckPadding(data_length41, sizeof(data_length41)) << std::endl;
      std::cout << (int)CheckPaddingNoVec(data_length40, sizeof(data_length40)) << std::endl;
      std::cout << (int)CheckPaddingNoVec(data_length41, sizeof(data_length41)) << std::endl;
  }

This has the output of 0 3 0 0 with the recommitted patch whereas the correct result is 0 0 0 0. https://gcc.godbolt.org/z/Mz9eeMscn

If we apply back

  --- a/llvm-project/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
  +++ b/llvm-project/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
  @@ -419,7 +419,12 @@ void VPlanTransforms::optimizeInductions
   
       VPScalarIVStepsRecipe *Steps = new VPScalarIVStepsRecipe(ID, BaseIV, Step);
       HeaderVPBB->insert(Steps, IP);
  -
  +    // If there are no vector users of IV, simply update all users to use Step
  +    // instead.
  +    if (!WideIV->needsVectorIV()) {
  +      WideIV->replaceAllUsesWith(Steps);
  +      continue;
  +    }
       // Update scalar users of IV to use Step instead. Use SetVector to ensure
       // the list of users doesn't contain duplicates.
       SetVector<VPUser *> Users(WideIV->user_begin(), WideIV->user_end());

Then it passes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123720