This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Change CTR clobber estimation for 128-bit floating types
ClosedPublic

Authored by qiucf on Jan 17 2022, 12:57 AM.

Details

Summary

Restrict fpext and fptrunc cast instructions to/from fp128.

We need a more complete fix to defer CTR clobber judgement after instruction selection. See D81353.

Diff Detail

Event Timeline

qiucf created this revision.Jan 17 2022, 12:57 AM
qiucf requested review of this revision.Jan 17 2022, 12:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2022, 12:57 AM
qiucf planned changes to this revision.Jan 17 2022, 8:24 AM
qiucf updated this revision to Diff 400857.Jan 18 2022, 8:26 AM

Fix check for fcmp because its result type is not float

qiucf updated this revision to Diff 401476.Jan 19 2022, 6:51 PM
qiucf edited the summary of this revision. (Show Details)
shchenz added inline comments.Jan 19 2022, 7:41 PM
llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
662

I think we are trying to handle fpext/fptrunc for fp128 type? If so, can we just explicitly handle them like:

} else if (isa<FPTruncInst>(J) && cast<FPTruncInst>(J)->getSrcTy()->getScalarType()->isFP128Ty()) {
  return true;
} else  if (isa<FPExtInst>(J) && cast<FPExtInst>(J)->getDestTy()->getScalarType()->isFP128Ty()) {
  return true;
}

Your patch seems not only change instructions FPTruncInst and FPExtInst. As you may have noted, ctr clobber check here is very sensitive especially for fp128/ppc_fp128. We met several issues before.

And have you checked with ppc_fp128 for the above two instructions FPTruncInst and FPExtInst? Can we expand them on PowerPC without potential writing ctr?

qiucf updated this revision to Diff 401535.Jan 20 2022, 12:35 AM
qiucf marked an inline comment as done.
qiucf added inline comments.
llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
662

Yes. Because of the structure of ppc_fp128:

  • fptrunc ppc_fp128 %a to double: nothing (just return first fpr)
  • fptrunc ppc_fp128 %a to float: xsrsp (trunc the first fpr to float)
  • fpext double %a to ppc_fp128: xxlxor (set the second part as zero)
  • fpext float %a to ppc_fp128: xxlxor (set the second part as zero)
shchenz accepted this revision as: shchenz.Jan 20 2022, 12:50 AM

Thanks for fixing this. LGTM

This revision is now accepted and ready to land.Jan 20 2022, 12:50 AM

I think that since we still keep getting issues with this after so many years and so many changes, we should just reject any operations on IBM double-double and fp128 until we refactor the code as suggested previously.

qiucf updated this revision to Diff 402214.Jan 22 2022, 7:06 AM

Restrict all operations with ppc_fp128/fp128 result type

This revision was landed with ongoing or failed builds.Jan 22 2022, 7:21 AM
This revision was automatically updated to reflect the committed changes.