Page MenuHomePhabricator

[IR] Add Freeze instruction
Needs ReviewPublic

Authored by aqjune on Jan 23 2017, 3:42 AM.

Details

Summary
  • 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.

Diff Detail

Event Timeline

aqjune created this revision.Jan 23 2017, 3:42 AM
aqjune removed a subscriber: mehdi_amini.
aqjune added a subscriber: llvm-commits.

Hi,

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

include/llvm-c/Core.h
3899

nit: please align the Name parameter properly.

nlopes added a subscriber: nlopes.Jan 24 2017, 1:08 AM
aqjune updated this revision to Diff 85541.Jan 24 2017, 1:46 AM
  • Add llvm-c test (test/Bindings/llvm-c/freeze.ll)
  • Update tools/llvm-c-test/echo.cpp so it can deal with Freeze instruction.
deadalnix edited edge metadata.Jan 24 2017, 3:29 AM

Awesome, as far as the C API goes, this is good. I'll let other the time to review everything else.

filcab added a subscriber: filcab.Jan 24 2017, 7:37 AM

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.

include/llvm/IR/IRBuilder.h
2377

Maybe omit the \brief?

2379

This is not being used. Maybe submit in a later patch, with a use for it?

include/llvm/IR/Instructions.h
5261

undef?

include/llvm/IR/PatternMatch.h
1338

Minor nit: Unsure if the usual pattern matching policy is to add the pattern only when you need it on when adding the instruction.

lib/Analysis/ValueTracking.cpp
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.
No need to add a comment stating what an if (isa...) return is doing.
Is there a plan for adding constants, etc? Maybe add a TODO tag here, so it jumps out if someone is grepping for those.

lib/AsmParser/LLParser.cpp
6740

Please test this error.

6742

Omit the , "" since it's the default arg anyway.

lib/Bitcode/Reader/BitcodeReader.cpp
5108

Omit the , "".

filcab added inline comments.Jan 24 2017, 10:37 AM
lib/AsmParser/LLParser.cpp
6740

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)

majnemer added inline comments.Jan 24 2017, 10:41 AM
lib/AsmParser/LLParser.cpp
6740

its not bad for them to be in both, the error messages are much nicer in the parser.

vsk added a subscriber: vsk.Jan 24 2017, 10:42 PM

Please move the bitcode round-trip in test/Bitcode/freeze.ll to test/Bitcode/compatibility.ll. This lets us test bitcode backwards-compatibility.

aqjune updated this revision to Diff 85760.Jan 25 2017, 8:26 AM
  • Modify typos, add more comments (including TODOs)
  • Add tests for parsing error
  • Moved test/Bitcode/freeze.ll to test/Bitcode/compatibility.ll.
aqjune marked 5 inline comments as done.Jan 26 2017, 1:28 AM
aqjune added inline comments.
include/llvm/IR/IRBuilder.h
2379

Hello. Thanks for your comment.
This function is going to be used in https://reviews.llvm.org/D29016, and this patch is important because it may reduce the number of unnecessarily inserted freezes.

include/llvm/IR/PatternMatch.h
1338

You're right - it is not used right now.
However every other values has its own match function currently, so I just followed convention..
If you think this is redundant now, I'll erase this.

lib/Analysis/ValueTracking.cpp
3917 ↗(On Diff #85760)

There's no plan to add other cases now.
I added TODOs, as you recommended.

lib/AsmParser/LLParser.cpp
6740

I added 3 test cases which checks whether this error message is printed out.

6742

I removed the second argument.

filcab added inline comments.Jan 27 2017, 6:51 AM
include/llvm/IR/IRBuilder.h
2379

That patch seems to have no instance of CreateFreezeAtDef. It's better to only add this function when it's going to be used.

include/llvm/IR/PatternMatch.h
1338

It's a minor nit. I'm ok with this being added now with the rest of the instruction stuff.

lib/Analysis/ValueTracking.cpp
3901 ↗(On Diff #85760)

This function is unused.

3917 ↗(On Diff #85760)

Maybe this?

// 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.
(we can even return isa<...>...;, but keeping the "itemization"-like code doesn't seem bad in my view).

lib/Bitcode/Reader/BitcodeReader.cpp
5108

If you don't test for an integer type, this will generate invalid IR. It's also not consistent with the parser.

lib/IR/Verifier.cpp
3963

Please add a test to unittests/IR/VerifierTest.cpp

test/Bitcode/freeze-vector.ll
4

No need to name the LINE variable. But you should be matching to make sure we get the correct line.
Something like (using the automatic @LINE variable):

CHECK: freeze-vector.ll:[[@LINE+2]]: ```...
The same for the other tests.
regehr added a subscriber: regehr.Jan 28 2017, 10:03 PM
aqjune updated this revision to Diff 86250.Jan 30 2017, 12:11 AM
aqjune marked 4 inline comments as done.Jan 30 2017, 12:19 AM
aqjune added inline comments.
include/llvm/IR/IRBuilder.h
2379

Oh, sorry, the patch was https://reviews.llvm.org/D29015 , not https://reviews.llvm.org/D29016.
I moved the CreateFreezeAtDef to https://reviews.llvm.org/D29015.

aqjune added inline comments.Jan 30 2017, 12:20 AM
include/llvm/IR/IRBuilder.h
2377

@filcab Other functions also have \brief , so I followed the convention.

filcab added a comment.Feb 2 2017, 9:19 AM

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.

include/llvm/IR/IRBuilder.h
2377

Consistency is ok by me. If we ever convert these, we can do it on all at once.

@filcab Thanks for your comments.

spatel added a subscriber: spatel.May 22 2017, 2:00 PM
aqjune updated this revision to Diff 106873.Jul 17 2017, 8:04 AM

Rebased to svn commit 308173

delcypher added inline comments.Aug 10 2017, 12:19 AM
lib/AsmParser/LLParser.cpp
6740

@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 ).

nlopes added inline comments.Aug 10 2017, 3:51 AM
lib/AsmParser/LLParser.cpp
6740

You're right that in the future it may be useful to freeze vectors of integers, at least.
This is just the first version of the patch. The intent was to fix loop unswitch, which doesn't require freezing vectors yet. As uses expand we can relax the condition.
We definitely didn't want to freeze floats; I don't know yet if that's needed or what does it mean.

delcypher added inline comments.Aug 13 2017, 4:33 AM
lib/AsmParser/LLParser.cpp
6740

Thanks for clarifying. I see the instruction is documented in https://reviews.llvm.org/D29121 so I left a minor comment there about this.

espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 9:48 AM
aqjune updated this revision to Diff 145376.May 5 2018, 11:00 AM

Rebased to svn commit 331585

aqjune updated this revision to Diff 217187.Mon, Aug 26, 10:13 AM

Rebase to trunk@369887 (August 26th, 2019)