Uploaded image for project: 'IMPALA'
  1. IMPALA
  2. IMPALA-6926

Don't use CodegenOptLevel::None on debug builds

    XMLWordPrintableJSON

Details

    • Improvement
    • Status: Open
    • Major
    • Resolution: Unresolved
    • Impala 2.12.0
    • None
    • Backend
    • ghx-label-1

    Description

      This code adds some unnecessary divergence between the codegen'd code for debug and release builds.

      #ifndef NDEBUG
        // For debug builds, don't generate JIT compiled optimized assembly.
        // This takes a non-neglible amount of time (~.5 ms per function) and
        // blows up the fe tests (which take ~10-20 ms each).
        opt_level = llvm::CodeGenOpt::None;
      #endif
      

      Ideally LLVM will generate correct code in both cases (and we won't accidentally have undefined behaviour in our code that could be exploited), but that isn't always a safe assumption, e.g. IMPALA-6764. The above #ifdef doesn't the effort required to get coverage for correctness of codegen'd code, since we need to run the same test for debug and release builds. More specifically, the random query generator is usually run against debug builds so that it will be able to hit DCHECKs, but that means we're not verifying the correctness of the generated code for a release build.

      We should re-evaluate the above code. A few observations:

      • The frontend tests shouldn't be doing codegen - almost all expressions being evaluated should be interpreted. So the comment may not be accurate any more.
      • The backend tests like expr-test may take significantly longer. I did a quick experiment which seemed to indicate that it added a few minutes. We could potentially leave the optimisation disabled for backend tests only
      • Query tests could spend a bit longer in codegen. We would have to quantify it but it probably isn't too bad.

      Attachments

        Activity

          People

            joemcdonnell Joe McDonnell
            tarmstrong Tim Armstrong
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated: