Page MenuHomePhabricator

Change ConstantFoldInstOperands to take Instruction instead of opcode and type. NFC.
ClosedPublic

Authored by mjacob on Jan 20 2016, 5:13 PM.

Details

Summary

The previous form, taking opcode and type, is moved to an internal
helper and the new form, taking an instruction, is a wrapper around this
helper.

Although this is a slight cleanup on its own, the main motivation is to
refactor the constant folding API to ease migration to opaque pointers.
This will be follow-up work.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 45468.Jan 20 2016, 5:13 PM
mjacob retitled this revision from to Change ConstantFoldInstOperands to take Instruction instead of opcode and type. NFC..
mjacob updated this object.
mjacob added a reviewer: eddyb.
mjacob added subscribers: llvm-commits, dblaikie.
eddyb accepted this revision.Jan 20 2016, 6:00 PM
eddyb edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 20 2016, 6:00 PM
mjacob closed this revision.Jan 20 2016, 10:37 PM

Hi Manuel,

Things like NewGVN don't always have instruction *'s to hand to ConstantFoldInstOperands.

For example, we create an expression that stores operands and opcodes and such (like GVN does!)

Before, we could pass the opcode and operands, and that worked great.

Now you've made it take the Instruction.

I don't see how this makes anything related to opaque pointers easier, maybe you could enlighten me?

Additionally, i can't see how use cases like mine are supposed to function - i have what was an instruction, but is now an operation and some operands.
I want to see if it will fold to a constant.

What API am i expected to use to do so?

Note also that the InstOperands API was *explicitly* designed to not require an instruction, to support the above use case. This was the difference between ConstantFoldInst and ConstantFoldInstOperands. It happens most users can pass it instructions, but that doesn't , IMHO make a lot of sense.

I want to request that you provide some way to not require the instruction, as the current API does (and many users use!), so that folks can use this without creating fake instructions to do so.

Hi Manuel,

Things like NewGVN don't always have instruction *'s to hand to ConstantFoldInstOperands.

For example, we create an expression that stores operands and opcodes and such (like GVN does!)

Before, we could pass the opcode and operands, and that worked great.

Now you've made it take the Instruction.

Sorry for making difficulties.

I don't see how this makes anything related to opaque pointers easier, maybe you could enlighten me?

The problem is getelementptr. With opaque pointers, we must have the instruction to get the source element type. Currently this is possible by getting the pointee type of the first (pointer) operand of the GEP. This won't be possibly with opaque pointers.

Additionally, i can't see how use cases like mine are supposed to function - i have what was an instruction, but is now an operation and some operands.
I want to see if it will fold to a constant.

What API am i expected to use to do so?

Note also that the InstOperands API was *explicitly* designed to not require an instruction, to support the above use case. This was the difference between ConstantFoldInst and ConstantFoldInstOperands. It happens most users can pass it instructions, but that doesn't , IMHO make a lot of sense.

ConstantFoldInstOperands constant folds the given instruction but with the specified operands. If I understand correctly, you still have the instruction available.

I want to request that you provide some way to not require the instruction, as the current API does (and many users use!), so that folks can use this without creating fake instructions to do so.

The problem with such an API is that it doesn't work with getelementptr (and not with comparison instructions, as it was not possible before this patch). I think that most use cases are covered by already existing functions.

  1. The user wants to fold an instruction but with other operands (e.g. because they were constant-folded by the user). ConstantFoldInstOperands should work here.
  2. The user wants to fold a binary operation or cast. I added functions for this in rL258389 and rL258390.

There is no API for the case when the user has a random instruction opcode but no instruction. I don't think that adding a function for this case which however doesn't work with getelementptr and compares is very helpful in this case.