- Define FreezeInst class in Instruction.def/.cpp/Instructions.h/.cpp
- Add support for FreezeInst to LLLexer/LLParser/BitcodeReader/BitcodeWriter The format is %x = freeze iN %v
- Add visitFreezeInst to InstVisitor.h
- Add isGuaranteedNotToBeUndefOrPoison in ValueTracking.h
- Verify that freeze instructions are used only with integer operands for now (Verifier.cpp).
- Add support for freeze instruction to llvm-c interface.
- Add m_Freeze in PatternMatch.
- Add CreateXXX functions to IRBuilder.
- Erase FreezeInst when lowering IR to SelDag.
Could you add a test case for the C API ? You can do so using the echo facility of llvm-c-test. See for instance: https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/Bindings/llvm-c/atomics.ll
nit: please align the Name parameter properly.
Just some minor nits.
You're adding some functions which will never be called. It might be easier to hold them off until the next patch (and have this one only add the instruction + necessary plumbing), or to at least post a follow-up patch on phabricator which shows usage for those functions, as that makes it easier to know it shouldn't be dead code for long.
Maybe omit the \brief?
This is not being used. Maybe submit in a later patch, with a use for it?
Minor nit: Unsure if the usual pattern matching policy is to add the pattern only when you need it on when adding the instruction.
|3917 ↗||(On Diff #85541)|
If we're just doing ConstantInt for now, why not just use the inner if (same for the FreezeInst)? Then change it after adding more, if the code ends up being too complex or has too many repeated tests.
Please test this error.
Omit the , "" since it's the default arg anyway.
Omit the , "".
Even better: Should this be in the verifier only? Otherwise you're checking for the same thing in two places, which is easy to get out of sync. (Would still require a test)
Hello. Thanks for your comment.
You're right - it is not used right now.
|3917 ↗||(On Diff #85760)|
There's no plan to add other cases now.
I added 3 test cases which checks whether this error message is printed out.
I removed the second argument.
That patch seems to have no instance of CreateFreezeAtDef. It's better to only add this function when it's going to be used.
It's a minor nit. I'm ok with this being added now with the rest of the instruction stuff.
|3901 ↗||(On Diff #85760)|
This function is unused.
|3917 ↗||(On Diff #85760)|
// TODO: Deal with other Constant subclasses. if (isa<ConstantInt>(C)) return true; return false;
I don't think the outer if is buying you anything for now. Anyone who adds more can deal with avoiding testing the same thing twice.
If you don't test for an integer type, this will generate invalid IR. It's also not consistent with the parser.
Please add a test to unittests/IR/VerifierTest.cpp
No need to name the LINE variable. But you should be matching to make sure we get the correct line.
CHECK: freeze-vector.ll:[[@LINE+2]]: ```... The same for the other tests.
Looks good on my end. But please wait for someone more familiar with IR to chime in and LGTM, since I'm not sure of all places that need to be changed when adding instructions.
Consistency is ok by me. If we ever convert these, we can do it on all at once.
@aqjune What's the motivation to restricting the freeze instruction to scalar integers?
For vector SSA registers that contain integers that we want to "freeze" this would force doing constructing a new vector by use of extractelement, freeze on the each scalar element and then insertelement to construct the new register. This seems a bit cumbersome.
I presume the reason for not allowing floating point operands is that none of the floating point instructions are supposed to produce poison? However consider the load instruction. That could load poisoned memory and the type could be a floating point type. There's an example of a poisoned load in the IR language reference ( https://llvm.org/docs/LangRef.html#poison-values ).
You're right that in the future it may be useful to freeze vectors of integers, at least.