[llvm-commits] [PATCH] Invariants (and Assume Aligned) - LLVM

[hfinkel at anl.gov]

  Thanks!

  Chandler was requested (on IRC) that I split this into 4 patches. I'll do that now, then I'll work on your comments.

   -Hal

  ----- Original Message -----
  > From: "Dmitri Gribenko" <gribozavr at gmail.com>
  > To: hfinkel at anl.gov
  > Cc: gribozavr at gmail.com, llvm-commits at cs.uiuc.edu, chandlerc at gmail.com
  > Sent: Wednesday, December 5, 2012 4:06:31 PM
  > Subject: Re: [PATCH] Invariants (and Assume Aligned) - LLVM
  >
  >
  >
  > ================
  > Comment at: docs/LangRef.html:9083-9085
  > @@ +9082,5 @@
  > +
  > +<p>The intrinsic allows the optimizer to assume that the provided
  > condition is
  > +   always true. No code is generated for this intrinsic, and
  > instructions that
  > +   contribute only to the provided condition are not used for code
  > generation.</p>
  > +</div>
  > ----------------
  > What about adding explicitly: "If the condition does not hold during
  > execution, the behavior is undefined"?
  >
  > ================
  > Comment at: include/llvm/Analysis/EphemeralValues.h:9-11
  > @@ +8,5 @@
  > +//===----------------------------------------------------------------------===//
  > +//
  > +// Calculate ephemeral values - those used only (indirectly) by
  > invariants.
  > +//
  > +//===----------------------------------------------------------------------===//
  > ----------------
  > Three slashes and '\file', please.
  >
  >
  > ================
  > Comment at: include/llvm/Analysis/EphemeralValues.h:35
  > @@ +34,3 @@
  > +  // Returns true if the provided value is ephemeral.
  > +  bool isEphemeralValue(Value *V) const {
  > +    return EphValues.count(V);
  > ----------------
  > Three slashes please.
  >
  > ================
  > Comment at: include/llvm/Analysis/InlineCost.h:110
  > @@ -108,1 +109,3 @@
  >      const DataLayout *TD;
  > +    // EphemeralValues if available, or null.
  > +    const EphemeralValues *EV;
  > ----------------
  > Three slashes...
  >
  > ================
  > Comment at: lib/Analysis/CaptureTracking.cpp:139-142
  > @@ -125,19 +138,6 @@
  >      }
  > -    case Instruction::Load:
  > -      // Loading from a pointer does not cause it to be captured.
  > -      break;
  > -    case Instruction::VAArg:
  > -      // "va-arg" from a pointer does not cause it to be captured.
  > -      break;
  > -    case Instruction::Store:
  > -      if (V == I->getOperand(0))
  > -        // Stored the pointer - conservatively assume it may be
  > captured.
  > -        if (Tracker->captured(U))
  > -          return;
  > -      // Storing to the pointee does not cause the pointer to be
  > captured.
  > -      break;
  >      case Instruction::BitCast:
  >      case Instruction::GetElementPtr:
  >      case Instruction::PHI:
  >      case Instruction::Select:
  >        // The original value is not captured via this if the new
  >        value isn't.
  > ----------------
  > Did you really intend to just move this piece of code a few lines
  > down?
  >
  > ================
  > Comment at: lib/Analysis/EphemeralValues.cpp:10-12
  > @@ +9,5 @@
  > +//===----------------------------------------------------------------------===//
  > +//
  > +// This file implements ephemeral value determination.
  > +//
  > +//===----------------------------------------------------------------------===//
  > ----------------
  > Three slashes and '\file', please.
  >
  > ================
  > Comment at: include/llvm/Analysis/EphemeralValues.h:44
  > @@ +43,3 @@
  > +  static char ID;
  > +  explicit EphemeralValues();
  > +
  > ----------------
  > Does this need 'explicit'?
  >
  > ================
  > Comment at: lib/Analysis/InlineCost.cpp:47
  > @@ -45,1 +46,3 @@
  >    const DataLayout *const TD;
  > +  // EphemeralValues if available, or null.
  > +  const EphemeralValues *const EV;
  > ----------------
  > Three slashes, please.
  >
  > ================
  > Comment at: lib/CodeGen/IntrinsicLowering.cpp:458
  > @@ -456,3 +457,3 @@
  >    case Intrinsic::var_annotation:
  >      break;   // Strip out annotate intrinsic
  >
  > ----------------
  > Please correct the comment (something like "strip out these
  > intrinsics")
  >
  > ================
  > Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5023
  > @@ -5021,3 +5022,3 @@
  >    case Intrinsic::var_annotation:
  >      // Discard annotate attributes
  >      return 0;
  > ----------------
  > Please correct the comment.
  >
  > ================
  > Comment at: lib/Transforms/Scalar/AlignmentInvProp.cpp:10-12
  > @@ +9,5 @@
  > +//===----------------------------------------------------------------------===//
  > +//
  > +// This file implements alignment invariant propagation.
  > +//
  > +//===----------------------------------------------------------------------===//
  > ----------------
  > Three slashes and '\file'
  >
  > ================
  > Comment at: lib/Transforms/Scalar/AlignmentInvProp.cpp:243
  > @@ +242,3 @@
  > +      unsigned(sizeof(unsigned) * CHAR_BIT - 1));
  > +    Alignment = std::min(1u << TrailingOnes,
  > +Value::MaximumAlignment);
  > +
  > ----------------
  > Is the '+' required here?
  >
  >
  > http://llvm-reviews.chandlerc.com/D150
  >

http://llvm-reviews.chandlerc.com/D150