Page MenuHomePhabricator

[RISCV] Fix ICE in isDesirableToCommuteWithShift
ClosedPublic

Authored by lenary on Jul 9 2019, 9:16 AM.

Details

Summary

There was an error being thrown from isDesirableToCommuteWithShift in
some tests. This was tracked down to the method being called before
legalisation, with an extended value type, not a machine value type.

In the case I diagnosed, the error was only hit with an instruction sequence
involving i24s in the add and shift. i24 is not a Machine ValueType, it is
instead an Extended ValueType which was causing the issue.

I have added a test to cover this case, and fixed the error in the callback.

Diff Detail

Repository
rL LLVM

Event Timeline

lenary created this revision.Jul 9 2019, 9:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 9:16 AM
asb accepted this revision.Jul 9 2019, 9:19 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 9 2019, 9:19 AM
This revision was automatically updated to reflect the committed changes.
apazos added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp
1010

Sam, I just noted when building compiler-rt (/compiler-rt/lib/builtins/addtf3.c) in debug mode, it crashes in this line.

The reason is ShiftedC1Int bitwidth is 128, but APInt getSExtValue expects a 64 bit quantity.

Can you take a look?

One solution is:

  • a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp

+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -1003,6 +1003,8 @@ bool RISCVTargetLowering::isDesirableToCommuteWithShift(

if (C1 && C2) {
  APInt C1Int = C1->getAPIntValue();
  APInt ShiftedC1Int = C1Int << C2->getAPIntValue();

+ if (ShiftedC1Int.getBitWidth() > 64)
+ return false;

Reduced test case:

define void @test(i128* %a) {
entry:

%load = load i128, i128* %a, align 16
%or = or i128 %load, 5192296858534827628530496329220096
%shl = shl i128 %or, 3
store i128 %shl, i128* %a, align 16

ret void
}

lenary marked an inline comment as done.Aug 12 2019, 5:05 AM
lenary added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVISelLowering.cpp
1010

Thanks for letting me know. I have prepared a patch for this, including a variant of your testcase (which I have verified currently causes the same ICE you're seeing). It is D66081