[PATCH] D26224: NewGVN

[Daniel Berlin via llvm-commits]

On Tue, Dec 13, 2016 at 4:01 PM, Hal Finkel via Phabricator <
reviews at reviews.llvm.org> wrote:

> hfinkel added inline comments.
>
>
> ================
> Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:65
> +      return true;
> +    // Compare etype for anything but load and store.
> +    if (getExpressionType() != ET_Load &&
> ----------------
>


> Please explain in this comment why you're not comparing the expression
> types for loads and stores.

Sigh.

If we didn't do coercion, i would say we should just clean this all up and
use a single MemoryLocationExpression or something (since we are really
value numbering the [memory location, memory state]), but  for coercion, we
need to be able to tell if they were loads or stores.


> This is also somewhat confusing because we've already compared the opcodes.

Yes, though the opcodes are 0 :P.


>
>
> ================
> Comment at: include/llvm/Transforms/Scalar/GVNExpression.h:183
> +  virtual bool equals(const Expression &Other) const override {
> +    const BasicExpression &OE = cast<BasicExpression>(Other);
> +    if (getOpcode() != OE.getOpcode())
> ----------------
> This looks a bit odd. Shouldn't you check the opcode equality first and
> then cast to BasicExpression?
>
> Yes.


>
> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:215
> +  // Debugging for how many times each block and instruction got
> processed.
> +  DenseMap<const Value *, unsigned> ProcessedCount;
> +
> ----------------
> Do we want to guard this with `#ifdef NDEBUG`?
>

Yes.


>
>
> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:460
> +// I, and see if it resulted in a simpler expression. If so, return
> +// that expression
> +// TODO: Once finished, this should not take an Instruction, we only
> ----------------
> Add period after expression.
>
>
> ================
> Comment at: lib/Transforms/Scalar/NewGVN.cpp:647
> +      new (ExpressionAllocator) CallExpression(CI->getNumOperands(), CI,
> HV);
> +  setBasicExpressionInfo(CI, E, B);
> +  return E;
> ----------------
> We also need to add operand bundles too for calls.
>
>
> https://reviews.llvm.org/D26224
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161213/371d8377/attachment.html>