Page MenuHomePhabricator

[Power9] Add __float128 support in the backend for bitcast to a i128
ClosedPublic

Authored by stefanp on Jul 18 2018, 1:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

stefanp created this revision.Jul 18 2018, 1:20 PM
syzaara commandeered this revision.Jul 24 2018, 1:30 PM
syzaara edited reviewers, added: stefanp; removed: syzaara.
syzaara updated this revision to Diff 157346.Jul 25 2018, 1:01 PM
stefanp commandeered this revision.Aug 8 2018, 11:00 AM
stefanp edited reviewers, added: syzaara; removed: stefanp.
lei added inline comments.Aug 10 2018, 6:15 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14129 ↗(On Diff #157346)

Can you add some general documentation for the function below to describe what it does?

nemanjai requested changes to this revision.Aug 13 2018, 7:27 AM

This is definitely going to need a test case.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
14137 ↗(On Diff #157346)

Why the call to getNode()? An SDValue is implicitly convertible to bool and will be false if this is a default-constructed one.

This revision now requires changes to proceed.Aug 13 2018, 7:27 AM
stefanp updated this revision to Diff 161557.Aug 20 2018, 1:46 PM

Added test case.
Address reviewers comments.

nemanjai added inline comments.Aug 27 2018, 7:46 AM
lib/Target/PowerPC/PPCISelLowering.cpp
14192 ↗(On Diff #161557)

Nit: indentation is off.

14195 ↗(On Diff #161557)

Perhaps for brevity and readability, combine this into the if:

if (SDValue CRTruncValue = DAGCombineTruncBoolExt(N, DCI))
  return CRTruncValue;
14203 ↗(On Diff #161557)

Nit: the more common way to declare references puts the ampersand immediately before the variable name.

14209 ↗(On Diff #161557)

Why not just Op0.getOpcode() and Op0.getOperand(0)? Here as well as below.

lib/Target/PowerPC/PPCInstrInfo.td
237 ↗(On Diff #161557)

I don't think it is OK to allow arbitrary integer types for the result nor arbitrary floating point types for operand 1. This really seems like it should use SDTCisVT. I realize that PPCbuild_fp128 has the same issue and we missed it, but I think we should favour fixing that one (in a separate patch) rather than making this one match.

test/CodeGen/PowerPC/f128-bitcast.ll
1 ↗(On Diff #161557)

I think once you add the BE portion to this patch, we should have another RUN to test that.

nemanjai requested changes to this revision.Sep 18 2018, 6:49 AM

Since there are unaddressed comments, I'm moving this off of my Must Review queue until they are addressed.

This revision now requires changes to proceed.Sep 18 2018, 6:49 AM
stefanp updated this revision to Diff 167802.Oct 1 2018, 12:22 PM

Address Review Comments

Instead of generating a special EXTRACT_FP128 node, could you bitcast to v2i64 and use EXTRACT_VECTOR_ELT instead? (I'm not that familiar with Power9, so it's possible that doesn't actually work for some reason.)

stefanp updated this revision to Diff 168330.Oct 4 2018, 11:12 AM

Removed the custom PPCISD node and it simplified the patch considerably.

Added conditions to handle bitconverts from f128 to a number of vector sizes (v2i64 down to v16i8).
Did not add the code for the bitconvert from the vector sizes to f128 because I have not been able to find an example to trigger the condition. It seems that the f128 is lowered into a store followed by a load.

efriedma added inline comments.Oct 4 2018, 11:21 AM
lib/Target/PowerPC/PPCISelLowering.cpp
14324 ↗(On Diff #168330)

80 columns.

I assume you want something like (uint32_t)DAG.getDataLayout().isBigEndian() here, not "0".

14330 ↗(On Diff #168330)

The second operand of the shift might not be a constant; getConstantOperandVal will crash in that case.

stefanp updated this revision to Diff 168502.Oct 5 2018, 12:16 PM

Addressed review comments.

Also added Big Endian to the test to cover that case.

nemanjai added inline comments.Oct 19 2018, 3:28 AM
lib/Target/PowerPC/PPCISelLowering.cpp
14314 ↗(On Diff #168502)

This logic is fairly deeply nested. Can you convert some of the conditions to early exits. This one certainly looks to fit the bill.

14331 ↗(On Diff #168502)

For clarity, please do the null pointer check first.

stefanp updated this revision to Diff 170207.Oct 19 2018, 9:00 AM

Simplified some of the conditionals (easier to read) and added an early exit.

nemanjai added inline comments.Oct 22 2018, 8:00 AM
lib/Target/PowerPC/PPCISelLowering.cpp
14331 ↗(On Diff #170207)

I think this can be further simplified and the duplicated code can be removed by essentially "looking through" the right shift and updating the element you need to extract.

stefanp updated this revision to Diff 170442.Oct 22 2018, 9:13 AM

Simplified the code further.

nemanjai accepted this revision.Oct 23 2018, 2:53 AM

I think the changes that are needed are clear enough that this doesn't require another review cycle. Approving this with the assumption that the required change will be made on the commit.

lib/Target/PowerPC/PPCISelLowering.cpp
14325 ↗(On Diff #170442)

This seems kind of misleading - makes the reader wonder what this "constant bit" is. I think something like // Switch the element number to extract. is a lot more descriptive.

14329 ↗(On Diff #170442)

There needs to be an else here that will return SDValue() since it is not OK to keep going if we have an SRL with a non-constant shift amount or a shift amount other than 64.
However, rather than an else I think it would be nicer to flip the condition and exit early...

// The right shift has to be by 64 bits.
if (!ConstNode || ConstNode->getZextValue() != 64)
  return SDValue();
14336 ↗(On Diff #170442)

The comment is redundant - basically just says that we're inside the if block.

This revision is now accepted and ready to land.Oct 23 2018, 2:53 AM
This revision was automatically updated to reflect the committed changes.