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

[Hal Finkel]

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
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory