This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Don't convert Loop to CTR Loop for fp128 BinaryOperator
ClosedPublic

Authored by ZhangKang on Jun 7 2020, 7:12 PM.

Details

Summary

If the instruction in the Loop use CTR, we won't convert loop to CTR loop.
For PPC BinaryOperator of fp128 will become libcall, we shouldn't convert loop to
CTR loop if the loop contain libCall.

But currently, in the PPCTTIImpl::mightUseCTR() function, we only deal with
BinaryOperator for ppc_fp128, don't deal with the fp128.

This bug has caused assertion error, when the loop we want to convert it to CTL loop
contains BinaryOperator for ppc_fp128 like %mul = fmul fp128 %0, %1.

Diff Detail

Unit TestsFailed

Event Timeline

ZhangKang created this revision.Jun 7 2020, 7:12 PM

Not a comment on this patch specifically, but in general, I'd recommend changing the PowerPC implementation of hardware loops to switch to an approach that allows making the final decision on whether to emit a hardware loop after isel. ARM does this.

Basically, if that target sets HWLoopInfo.CounterInReg to true in isHardwareLoopProfitable, the HardwareLoops pass emits slightly different intrinsics; the alternate intrinsics are more straightforward to lower to a regular compare/branch. Then, if the target decides after isel that a hardware loop is illegal or unprofitable, it can easily fall back. That way, we can avoid this endless stream of bugs: if isHardwareLoopProfitable misses some obscure construct that requires clobbering CTR, it doesn't matter.

Not a comment on this patch specifically, but in general, I'd recommend changing the PowerPC implementation of hardware loops to switch to an approach that allows making the final decision on whether to emit a hardware loop after isel. ARM does this.

Basically, if that target sets HWLoopInfo.CounterInReg to true in isHardwareLoopProfitable, the HardwareLoops pass emits slightly different intrinsics; the alternate intrinsics are more straightforward to lower to a regular compare/branch. Then, if the target decides after isel that a hardware loop is illegal or unprofitable, it can easily fall back. That way, we can avoid this endless stream of bugs: if isHardwareLoopProfitable misses some obscure construct that requires clobbering CTR, it doesn't matter.

Cannot agree with this suggestion any more. We are struggling to sync the logic with isel that is hard to maintain.

shchenz accepted this revision.Jun 16 2020, 6:55 PM

I think it is a good idea to decide hardware loop or not after ISEL where IR operations with a certain type will be lowered.
However, this improvement should be aside from this bug fix patch.

This patch LGTM

This revision is now accepted and ready to land.Jun 16 2020, 6:55 PM
This revision was automatically updated to reflect the committed changes.