This is an archive of the discontinued LLVM Phabricator instance.

Differential D70794

[GlobalISel] Fix compiler crash lowering G_LOAD in AArch64.
ClosedPublic

Authored by drodriguez on Nov 27 2019, 11:20 AM.

Details

Reviewers
qcolombet
aemerson
Commits
rG28f5ad5801d4: [GlobalISel] Fix compiler crash lowering G_LOAD in AArch64.
Summary

When lowering G_LOAD in AArch64, a G_ANYEXT could have been inserted,
even if we were loading from a bigger stored and requesting a smaller
size.

The solution is changing the fixed usage of G_ANYEXT to a conditional
usage of TRUNC or G_ANYEXT depending on the size of the source and the
destination. This seems to avoid the crash in the legalizer.

A test case is included.

Diff Detail

Event Timeline

drodriguez created this revision.Nov 27 2019, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2019, 11:20 AM
Herald added subscribers: llvm-commits, Petar.Avramovic, volkan and 3 others. · View Herald Transcript
plotfi added a comment.Nov 28 2019, 2:03 PM

LGTM assuming @qcolombet or @aemerson have no objections.

aemerson added a comment.Dec 2 2019, 1:45 PM

Could you upload a diff with more context (-U9999)?

plotfi added a comment.Dec 2 2019, 2:25 PM
In D70794#1765975, @aemerson wrote:

Could you upload a diff with more context (-U9999)?

Agreed. More context would be good.

drodriguez updated this revision to Diff 231797.Dec 2 2019, 4:03 PM

Provide more context. Rebase on top of master, just in case.

aemerson added inline comments.Dec 2 2019, 5:04 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
15

This looks wrong. The resulting code is a load that generates an s10? But that value is then truncated down from s16 to s10? Shouldn't the load be 16 bit?

qcolombet added inline comments.Dec 2 2019, 5:32 PM
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
2

Could you generate the checks using the update_mir_test thing?
Like @aemerson pointed out the checks look wrong and I am wondering if you found another bug, or just forgot to update the test.

drodriguez marked 2 inline comments as done.Dec 2 2019, 6:25 PM
drodriguez added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir
2

I didn’t know about the tool. I will generate the checks with it.

15

I think I forgot to run the FileCheck at the end to correct this things. I will send a new diff asap. Sorry for the confusion.

drodriguez updated this revision to Diff 231808.Dec 2 2019, 6:27 PM

Generate the checks in the test with update_mir_test_checks.py. Check with FileCheck that the test actually passes.

aemerson accepted this revision.Dec 3 2019, 1:39 PM

I think the test can be simplified a bit to not have the extra blocks and just testing the relevant instructions, otherwise LGTM.

This revision is now accepted and ready to land.Dec 3 2019, 1:39 PM
drodriguez updated this revision to Diff 232024.Dec 3 2019, 6:47 PM

Reduced the test case.

drodriguez added a comment.Dec 3 2019, 6:49 PM

@aemerson: Thank you very much for accepting. I think I reduced the test case correctly. I needed to use the result of the G_LOAD, or the code was not being generated with the necessary G_TRUNC. I didn't imagine that it was as easy as returning the result. I hope the new test is better than the last.

Also, I don't have permission to merge, what's the next step for me to do? Thanks.

aemerson added a comment.Dec 3 2019, 7:35 PM
In D70794#1768237, @drodriguez wrote:

@aemerson: Thank you very much for accepting. I think I reduced the test case correctly. I needed to use the result of the G_LOAD, or the code was not being generated with the necessary G_TRUNC. I didn't imagine that it was as easy as returning the result. I hope the new test is better than the last.

Also, I don't have permission to merge, what's the next step for me to do? Thanks.

I can commit it for you later, after a few patches you can request commit access for yourself.

Closed by commit rG28f5ad5801d4: [GlobalISel] Fix compiler crash lowering G_LOAD in AArch64. (authored by aemerson). · Explain WhyDec 4 2019, 5:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
CodeGen/
GlobalISel/
c/
llvm/
lib/
CodeGen/
GlobalISel/
2 lines
llvm/
test/
CodeGen/
AArch64/
GlobalISel/
45 lines

Diff 231808

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

Show First 20 Lines • Show All 2,103 Lines • ▼ Show 20 Lines case TargetOpcode::G_ZEXTLOAD: {
if (DstTy.isScalar()) { if (DstTy.isScalar()) {
Register TmpReg = Register TmpReg =
MRI.createGenericVirtualRegister(LLT::scalar(MMO.getSizeInBits())); MRI.createGenericVirtualRegister(LLT::scalar(MMO.getSizeInBits()));
MIRBuilder.buildLoad(TmpReg, PtrReg, MMO); MIRBuilder.buildLoad(TmpReg, PtrReg, MMO);
switch (MI.getOpcode()) { switch (MI.getOpcode()) {
default: default:
llvm_unreachable("Unexpected opcode"); llvm_unreachable("Unexpected opcode");
case TargetOpcode::G_LOAD: case TargetOpcode::G_LOAD:
MIRBuilder.buildAnyExt(DstReg, TmpReg); MIRBuilder.buildExtOrTrunc(TargetOpcode::G_ANYEXT, DstReg, TmpReg);
break; break;
case TargetOpcode::G_SEXTLOAD: case TargetOpcode::G_SEXTLOAD:
MIRBuilder.buildSExt(DstReg, TmpReg); MIRBuilder.buildSExt(DstReg, TmpReg);
break; break;
case TargetOpcode::G_ZEXTLOAD: case TargetOpcode::G_ZEXTLOAD:
MIRBuilder.buildZExt(DstReg, TmpReg); MIRBuilder.buildZExt(DstReg, TmpReg);
break; break;
} }
▲ Show 20 Lines • Show All 2,200 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/legalize-load-trunc.mir

  • This file was added.
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -march=aarch64 -run-pass=legalizer %s -o - -verify-machineinstrs | FileCheck %s
qcolombetUnsubmitted
Not Done
ReplyInline Actions

Could you generate the checks using the update_mir_test thing?
Like @aemerson pointed out the checks look wrong and I am wondering if you found another bug, or just forgot to update the test.

qcolombet: Could you generate the checks using the `update_mir_test` thing? Like @aemerson pointed out the…
drodriguezAuthorUnsubmitted
Done
ReplyInline Actions

I didn’t know about the tool. I will generate the checks with it.

drodriguez: I didn’t know about the tool. I will generate the checks with it.
---
name: test_load_trunc
stack:
- { id: 0, type: default, offset: 0, size: 2,
alignment: 2, stack-id: default, callee-saved-register: '', callee-saved-restored: true,
debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
body: |
; CHECK-LABEL: name: test_load_trunc
; CHECK: bb.0.entry:
; CHECK: successors: %bb.1(0x40000000), %bb.2(0x40000000)
; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %stack.0
; CHECK: [[LOAD:%[0-9]+]]:_(s16) = G_LOAD [[FRAME_INDEX]](p0) :: (load 2)
; CHECK: [[TRUNC:%[0-9]+]]:_(s10) = G_TRUNC [[LOAD]](s16)
aemersonUnsubmitted
Not Done
ReplyInline Actions

This looks wrong. The resulting code is a load that generates an s10? But that value is then truncated down from s16 to s10? Shouldn't the load be 16 bit?

aemerson: This looks wrong. The resulting code is a load that generates an s10? But that value is then…
drodriguezAuthorUnsubmitted
Done
ReplyInline Actions

I think I forgot to run the FileCheck at the end to correct this things. I will send a new diff asap. Sorry for the confusion.

drodriguez: I think I forgot to run the `FileCheck` at the end to correct this things. I will send a new…
; CHECK: [[TRUNC1:%[0-9]+]]:_(s1) = G_TRUNC [[TRUNC]](s10)
; CHECK: G_BRCOND [[TRUNC1]](s1), %bb.1
; CHECK: G_BR %bb.2
; CHECK: bb.1:
; CHECK: successors: %bb.3(0x80000000)
; CHECK: G_BR %bb.3
; CHECK: bb.2:
; CHECK: successors: %bb.3(0x80000000)
; CHECK: bb.3:
; CHECK: RET_ReallyLR
bb.0.entry:
successors: %bb.2(0x40000000), %bb.3(0x40000000)
%0:_(p0) = G_FRAME_INDEX %stack.0
%1:_(s10) = G_LOAD %0(p0) :: (load 2)
%2:_(s1) = G_TRUNC %1(s10)
G_BRCOND %2(s1), %bb.2
G_BR %bb.3
bb.2:
successors: %bb.4(0x80000000)
G_BR %bb.4
bb.3:
successors: %bb.4(0x80000000)
bb.4:
RET_ReallyLR
...