This is an archive of the discontinued LLVM Phabricator instance.

RISCV: clean up target expression handling
ClosedPublic

Authored by compnerd on Jun 17 2021, 10:19 AM.

Details

Summary

The target specific expression handling was slightly regressed by
bbea64250f65480d787e1c5ff45c4de3ec2dcda8. This restores the proper
sub-expression evaluation to allow for constant folding within the
expression. We explicitly discard the layout and assembler when
evaluating the expression to avoid any symbolic computation and instead
using the evaluateAsRelocatable to canonicalise and constant fold
only.

We can also simplify the expression handling - none of the target
variants support symbolic difference. This simplifies the logic for
that and adds additional tests to ensure that we do not accidentally
regress here in the future.

Diff Detail

Event Timeline

compnerd created this revision.Jun 17 2021, 10:19 AM
compnerd requested review of this revision.Jun 17 2021, 10:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2021, 10:19 AM
MaskRay added inline comments.Jun 17 2021, 10:24 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
95

IIRC passing Layout can fold more expressions. In this case is Layout harmful?

llvm/test/MC/RISCV/expressions.s
7

Better to expand :[[#]]: to an integer. When testing parsing errors, it is useful to have precise column numbers.
Users need such information so it should be properly tested.

compnerd added inline comments.Jun 17 2021, 10:26 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
95

Yes; I guess I should add a comment here as well. I put it in the commit message - this is intentional to prevent any type of expression folding involving symbols. We want the symbolic differences to be preserved.

compnerd updated this revision to Diff 352787.Jun 17 2021, 10:46 AM

Address feedback from @MaskRay

compnerd marked an inline comment as done.Jun 17 2021, 10:46 AM
MaskRay accepted this revision.Jun 17 2021, 11:05 AM

Looks great! One nit

llvm/test/MC/RISCV/expressions.s
8

The line numbers should keep using [[#@LINE-1]]` or +1.

Otherwise adding lines will cause unneeded churn.

This revision is now accepted and ready to land.Jun 17 2021, 11:05 AM
This revision was landed with ongoing or failed builds.Jun 17 2021, 1:37 PM
This revision was automatically updated to reflect the committed changes.