Page MenuHomePhabricator

[IR] Strawman for dedicated FNeg IR instruction
ClosedPublic

Authored by cameron.mcinally on Oct 30 2018, 11:01 AM.

Details

Summary

This is a first whack at a dedicated FNeg IR instruction...

I've over-engineered the UnaryOperator class a little, with the intention of adding more Unary operations in the future. E.g. integer Neg (and maybe Abs, Copysign, etc.).

I don't work on some of these parts of LLVM, so I have some questions for the group. The majority are about reordering enums without breaking public interfaces. I'll comment on this code inline shortly.

Also, changing one of the operator enums appears to have exposed a bug in the MergeFuctions pass. I'll also comment on this inline.

Diff Detail

Repository
rL LLVM

Event Timeline

include/llvm/Bitcode/LLVMBitCodes.h
346 ↗(On Diff #171709)

It wasn't clear to me if it was safe to reorder these values. Any thoughts on this?

479 ↗(On Diff #171709)

Same thing here about organizing the enum members.

lib/AsmParser/LLParser.cpp
3322 ↗(On Diff #171709)

The lexer/parser is another area that I'm not too familiar with. A thorough review would be appreciated.

lib/Bitcode/Reader/BitcodeReader.cpp
2346 ↗(On Diff #171709)

I'm not that familiar with the Bitcode format. Any suggestions on the best way to produce a test case for this? Or is it just the obvious, produce bitcode and make sure it works as expected?

cameron.mcinally added inline comments.
include/llvm-c/Core.h
141 ↗(On Diff #171709)

Reordering these values appears to have exposed a bug in the MergeFunctions pass (*I think*). It seems that the hash accumulator can overflow with these larger numbers. Although, I could be mistaken.

@dyatkovskiy

include/llvm-c/Core.h
141 ↗(On Diff #171709)

Ah, sorry. This was the wrong bit of code. Will correct...

include/llvm/IR/Instruction.def
221 ↗(On Diff #171709)

Reordering these values appears to have exposed a bug in the MergeFunctions pass (*I think*). It seems that the hash accumulator can overflow with these larger numbers. Although, I could be mistaken.

@dyatkovskiy

with the intention of adding more Unary operations in the future. E.g. integer Neg (and maybe Abs, Copysign, etc.).

While there are very clear reasons for FNeg to exist, this isn't so true for any of the others listed here..

include/llvm-c/Core.h
64 ↗(On Diff #171709)

This strongly suggests that you can not reorder/renumber the existing enumerators.

lib/Bitcode/Writer/BitcodeWriter.cpp
521 ↗(On Diff #171709)

There is no integer neg opcode to handle.

with the intention of adding more Unary operations in the future. E.g. integer Neg (and maybe Abs, Copysign, etc.).

While there are very clear reasons for FNeg to exist, this isn't so true for any of the others listed here..

I don't disagree with that. On the other hand, these do seem like common enough operators to justify retiring the intrinsics and creating IR operators for them.

Also, integer Neg may have a stronger case for a dedicated IR instruction than the others. It's currently implemented as -0-x (Sub), which is pretty weird.

lib/Bitcode/Writer/BitcodeWriter.cpp
521 ↗(On Diff #171709)

Right, integer Neg is handled the same as FNeg is now. I.e. -0-x (Sub). I'm suggesting that integer Neg should also have a dedicated IR instruction. Although, that case is not as clear cut as it is for FNeg.

with the intention of adding more Unary operations in the future. E.g. integer Neg (and maybe Abs, Copysign, etc.).

While there are very clear reasons for FNeg to exist, this isn't so true for any of the others listed here..

I don't disagree with that. On the other hand, these do seem like common enough operators to justify retiring the intrinsics and creating IR operators for them.

From what i know, the bar isn't "common enough operator", the bar is more like "too easy to break the existing multi-op pattern".
Funnel shift (rotates) has recently met it, and was added as an operator.
But anyway.

Also, integer Neg may have a stronger case for a dedicated IR instruction than the others. It's currently implemented as -0-x (Sub), which is pretty weird.

dexonsmith added inline comments.Oct 30 2018, 1:15 PM
include/llvm/Bitcode/LLVMBitCodes.h
346 ↗(On Diff #171709)

No. Reordering will break bitcode compatibility.

lib/Bitcode/Reader/BitcodeReader.cpp
2346 ↗(On Diff #171709)

New IR we usually test in test/Assembler. You should test double-round-tripping through bitcode (llvm-as | llvm-dis | llvm-as | llvm-dis) and pass through verify-uselistorder. Have a look at existing tests there.

I don't think it applies here, but in cases where you need to upgrade existing bitcode to something new, we check in tiny bitcode files generated with a known released version of llvm and test that it upgrades correctly in test/Bitcode.

lib/IR/Verifier.cpp
3011 ↗(On Diff #171709)

Just caught this bad copy-and-paste. Will address in next revision...

Needs some dedicated assembler tests. In particular I would like to see parsing tests with the fast math flags

include/llvm/Bitcode/LLVMBitCodes.h
479 ↗(On Diff #171709)

Any guesses for why 14 wasn't used before?

lib/Bitcode/Writer/BitcodeWriter.cpp
521 ↗(On Diff #171709)

I would remove this comment. I don't think there's a reason to have an integer neg

Updating diff to add test/Assembler tests. This includes both fastmath flags and double-round-tripping tests.

Note that the constant folding test added needs work since FNeg is not currently being folded.

@arsenm @dexonsmith

cameron.mcinally marked 7 inline comments as done.Oct 31 2018, 2:07 PM
cameron.mcinally added inline comments.
include/llvm/Bitcode/LLVMBitCodes.h
479 ↗(On Diff #171709)

It looks like @void removed it as part of the "unwind removal" project. There's not a whole lot of history beyond that attached. The commit is r149911, and the message says:

"[unwind removal] Remove a the obsolete 'unwind' enum value."

That line was the only one that changed in this commit.

arsenm added inline comments.Nov 1 2018, 7:56 AM
include/llvm-c/Core.h
111 ↗(On Diff #172014)

If you're renumbering all of these anyway, might as well fix AddrSpaceCast to be consecutive?

lib/AsmParser/LLParser.cpp
6122–6123 ↗(On Diff #172014)

Put these on separate lines?

test/Assembler/fast-math-flags.ll
81 ↗(On Diff #172014)

Looks like extra whitespace in the test and output (I would hope this test would use -strict-whitespace, but it looks like this is an issue with the rest of the tests here as well)

include/llvm-c/Core.h
111 ↗(On Diff #172014)

Ah, forgot about these. Are we all in agreement that it's safe to reorder these values? I don't fully understand the library interactions, so I'm not sure.

I'll go ahead and implement your suggestion. If it turns out it's not safe, I'll revert the change later.

Reorder LLVMAddrSpaceCast as @arsenm suggested.

It's still not clear whether it's safe to reorder these values though.

cameron.mcinally marked 2 inline comments as done.Nov 1 2018, 8:39 AM
cameron.mcinally added inline comments.
test/Assembler/fast-math-flags.ll
81 ↗(On Diff #172014)

Created D53981 to correct the general whitespace problem.

Remove extra whitespace from test/Assembler/fast-math-flags.ll.

cameron.mcinally marked 2 inline comments as done.Nov 1 2018, 10:27 AM
jyknight added inline comments.Nov 1 2018, 11:26 AM
include/llvm-c/Core.h
64 ↗(On Diff #171709)

You should _NOT_ renumber the existing constants in the llvm-c/* APIs. External users are depending on them being stable.

(Adding a comment indicating that while you're in here would be good).

include/llvm-c/Core.h
64 ↗(On Diff #171709)

That settles it! Thanks, James.

So LLVMAddrSpaceCast is grouped correctly, but the value is out of order. Is that the correct way to add a new member?

Or should I just add FNeg to the end of the list?

Don't reorder the members in llvm-c/Core.h as suggested by @jyknight.

cameron.mcinally marked 3 inline comments as done.Nov 1 2018, 12:05 PM
arsenm added inline comments.Nov 1 2018, 1:05 PM
lib/IR/Verifier.cpp
3011 ↗(On Diff #171709)

Missing failing verifier tests

lib/IR/Verifier.cpp
3011 ↗(On Diff #171709)

I'm not familiar with the Verifier tests, so please excuse my naivete...

Is it possible to trip these asserts with llvm-as? It looks like the parser will catch an incorrect type on the operand before we get to these asserts.

Add a comment about not reordering the llvm-c/Core.h enum values, as @jyknight suggested.

Sorry, this slipped my mind yesterday...

Add newlines to lib/AsmParser/LLParser.cpp, as suggested by @arsenm.

cameron.mcinally marked an inline comment as done.Nov 7 2018, 1:25 PM
arsenm added a comment.Nov 7 2018, 1:36 PM

Bitcode compatibility tests are missing?

Bitcode compatibility tests are missing?

There's some BC coverage with the double-round-tripping test/Assembler tests that were added (I think?).

Could you point me to an example test in test/Bitcode, so that I know what I should be exercising? I don't see many tests that exercise the FP operations. Well, not directly at least.

arsenm added a comment.Nov 7 2018, 2:11 PM

Bitcode compatibility tests are missing?

There's some BC coverage with the double-round-tripping test/Assembler tests that were added (I think?).

Could you point me to an example test in test/Bitcode, so that I know what I should be exercising? I don't see many tests that exercise the FP operations. Well, not directly at least.

test/Bitcode/compatibility.ll

Ah, I think I see some problems with llvm-bcanalyzer. Will address...

<UnknownCode14 abbrevid=5 op0=1 op1=0/>

Add two test/Bitcode tests and address one llvm-bcanalyzer issue, as suggested by @arsenm.

arsenm added inline comments.Nov 7 2018, 3:00 PM
test/Bitcode/compatibility.ll
1004 ↗(On Diff #173035)

Should also test the fast math flags since you had to handle those

Add bitcode tests for fastmath flags, as @arsenm suggested. Thanks for bearing with me, Matt.

arsenm accepted this revision.Nov 7 2018, 3:36 PM

LGTM

This revision is now accepted and ready to land.Nov 7 2018, 3:36 PM

Wow, that's great. Thanks, Matt!

It was mentioned earlier, but changing the enum values in Instruction.def appears to have exposed a bug in the MergeFunctions pass. I'll try to work that out with the author(s) of that pass and then commit this (assuming, of course, there are no changes to this patch).

Update test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll to account for changes in the Instruction.def enum values.

This test was also touched the last time these enum values were modified, in r255522. So, it appears to just be a fragile test. I'm simply reverting the change to that test from r255522.

Apologies for the last minute revision. This patch now tests cleanly and is ready to land upon approval.

majnemer added inline comments.
include/llvm/Bitcode/LLVMBitCodes.h
479 ↗(On Diff #171709)

I don't think we should reuse the function code number.

Rebase and create new function code number, as suggested by @majnemer.

cameron.mcinally marked 4 inline comments as done.Nov 12 2018, 9:22 AM

David also gave a LGTM for the call-and-invoke-with-ranges.ll change off-line. If he's okay with the function code number change in LLVMBitCodes.h, I'll go ahead and commit this patch.

Thanks again, David.

@majnemer has approved the changes offline. Will commit this patch imminently...

Closed by commit rL346774: [IR] Add a dedicated FNeg IR Instruction (authored by mcinally, committed by ). · Explain WhyNov 13 2018, 10:18 AM
This revision was automatically updated to reflect the committed changes.

I just realized the LangRef changes are missing

I just realized the LangRef changes are missing

Bah! I must have inadvertently dropped them at some point along the way. I'll create a new patch now for it...

I just realized the LangRef changes are missing

Just FYI that I am working on this. Our in-house machines don't support the Sphinx package, so I'm having trouble generating the docs. Trying to find a workaround now...