Page MenuHomePhabricator

[RISCV] Clean up parsing fence arguments
AcceptedPublic

Authored by jrtc27 on Mar 6 2021, 11:47 AM.

Details

Summary

Currently this is done rather hackily by letting it be parsed as an
MCSymbolRefExpr whose name we then validate. The logic is also split
across isFenceArg and addFenceArgOperands as a result. Instead, use a
custom parser so we look at the identifier token directly and construct
the immediate at the same time. As well as being cleaner, this also
allows us to give better error messages.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > Polly.Isl/CodeGen::param_div_div_div_2.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen -polly-codegen-verify -polly-scops -analyze < /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/param_div_div_div_2.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/param_div_div_div_2.ll
40 msx64 debian > Polly.Isl/CodeGen::scop_expander_insert_point.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen -polly-codegen-verify -polly-codegen -S -polly-invariant-load-hoisting=true < /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/scop_expander_insert_point.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/scop_expander_insert_point.ll

Event Timeline

jrtc27 created this revision.Mar 6 2021, 11:47 AM
jrtc27 requested review of this revision.Mar 6 2021, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2021, 11:47 AM
craig.topper added inline comments.Mar 7 2021, 10:18 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1688

I see this SMLoc::getFromPointer(S.getPointer() - 1) repeated a lot in the assembly parser. Is this doing something I don't understand and setting a valid end location or are we just frequently setting an invalid end location?

jrtc27 added inline comments.Mar 7 2021, 10:28 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1688

Hm, indeed, this seems to be a consistent misunderstanding in the parser here. Normally you'd lex and _then_ get E based on the _new_ location, but this creates a -1-sized range. The -1 itself is also odd, though seems to be rather pervasive across the backends; SMRange is meant to be half-open, so the -1 is already accounted for.

craig.topper added inline comments.Mar 7 2021, 10:32 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1688

You don't even need to Lex the next token. There's a getEndLoc() on the Token class.

jrtc27 added inline comments.Mar 7 2021, 11:34 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1688

In this case, yeah, though some cases in this file are more complex and so would likely need to use that kind of pattern instead.

luismarques accepted this revision.Mar 15 2021, 3:01 AM

LGTM.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1706–1717

Keep the original in-line formatting?

llvm/test/MC/RISCV/rv32i-invalid.s
5–7

letters -> operand letters?

9

ditto.

This revision is now accepted and ready to land.Mar 15 2021, 3:01 AM
asb accepted this revision.Mar 18 2021, 8:38 AM

Great cleanup, thanks for this.