This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support constant operand for la and lla pseudoinstruction.
ClosedPublic

Authored by garvitgupta08 on May 8 2023, 11:44 AM.

Details

Summary

This patch improves compatibility with GNU assembler by adding support for
constant immediate in la and lla pseudo instruction, and expanding it in the
same way as we currently expands li pseudo instruction.

Links to discussion related to the above issue in the community -
https://github.com/riscv-non-isa/riscv-arch-test/issues/105
https://github.com/riscv-non-isa/riscv-arch-test/issues/108
https://github.com/riscv-non-isa/riscv-arch-test/issues/106

Diff Detail

Event Timeline

garvitgupta08 created this revision.May 8 2023, 11:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 11:44 AM
garvitgupta08 requested review of this revision.May 8 2023, 11:44 AM
garvitgupta08 edited the summary of this revision. (Show Details)May 8 2023, 11:46 AM
garvitgupta08 retitled this revision from [RISCV] Support constant immediate for la pseudoinstruction to [RISCV] Support constant immediate for la pseudo instruction.May 8 2023, 11:51 AM
garvitgupta08 edited the summary of this revision. (Show Details)
garvitgupta08 edited the summary of this revision. (Show Details)

Fixing premerge checks

garvitgupta08 edited the summary of this revision. (Show Details)

Fixing Premerge checks

Using AUIPC to generate an absolute address (which is how GNU people seem to justify this ugly ability) sounds wrong

garvitgupta08 updated this revision to Diff 521304.EditedMay 11 2023, 7:31 AM

Using AUIPC to generate an absolute address (which is how GNU people seem to justify this ugly ability) sounds wrong

To be on the same page - GCC expands this instruction as what it does with li i.e. GCC expands it into lui + addi pair. But since spec mentions auipc + addi, therefore I have used that.

Using AUIPC to generate an absolute address (which is how GNU people seem to justify this ugly ability) sounds wrong

To be on the same page - GCC expands this instruction as what it does with li i.e. GCC expands it into lui + addi pair. But since spec mentions auipc + addi, therefore I have used that.

GCC doesn't do anything with assembly, it's binutils's assembler that expands pseudos. And the spec says auipc+addi or auipc+l[wd], because it's in the context of getting a symbol's address, it doesn't document being able to pass an immediate argument at all, which does not make sense to generate with auipc + addi. So don't; it's completely broken. At which point, just reuse the expansion for li.

You'll also need to see which of la, lla and lga support an immediate in order to determine where this needs to be handled.

And please upload your diffs with full context per https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch.

asb added a comment.EditedMay 17 2023, 4:46 AM

Honestly I'd be leaning towards compatibility with gas in this case. la with integer arguments may not seem elegant, but it's consistent with flexibility afforded elsewhere (e.g. add with integer arguments).

But yes, if you want to propose this please do use the same expansion that binutils/gas does (essentially having la of an immediate as an alias of li as I understand it).

evandro removed a subscriber: evandro.May 17 2023, 3:51 PM
garvitgupta08 updated this revision to Diff 523405.EditedMay 18 2023, 9:15 AM

Change la with constant immediate expand to lui + addi - same as what GNU assembler do. (Essentially making la an alias of li)

garvitgupta08 edited the summary of this revision. (Show Details)May 18 2023, 9:19 AM
garvitgupta08 edited the summary of this revision. (Show Details)May 18 2023, 10:21 AM

Hi @jrtc27 @asb, I have pushed new changes in accordance with your previous comments. Can you please check and provide an update on the same? Thanks

jrtc27 added inline comments.May 22 2023, 9:13 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1635

Size = 32 probably warrants a comment pointing back at PseudoLI's explanation

mayLoad should be 0?

1637

Why can't we just reuse ixlenimm_li?

1638

Indentation

1640

Why? We expand li during parsing, so we should never be trying to print it.

garvitgupta08 marked 3 inline comments as done.May 22 2023, 10:06 PM
garvitgupta08 added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1637

Because the DiagnosticType in class ImmXLenAsmOperand is !strconcat("Invalid", Name), and since the error message for li and la is different (because la supports both symbol and constant immediate) therefore I had to create a new ixlenimm_la type to support that.

garvitgupta08 marked an inline comment as done.May 22 2023, 10:13 PM

Hi @jrtc27, I have pushed new changes per your comments. Please have a look.

An update on the new changes will be appreciated!. Thanks

jrtc27 added inline comments.Jun 1 2023, 4:25 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1641

I don't understand why you need a new pseudo rather than teaching the existing one to also accept an integer

1644

You marked my "Why? We expand li during parsing, so we should never be trying to print it." comment as done yet this is still here. I do not see why we would ever want it.

Addressed the comments

garvitgupta08 added inline comments.Jun 8 2023, 10:05 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
1641

I did this in the revision https://reviews.llvm.org/D150133?id=521304. Below is the diff for the same but I believe that having a separate defintion for supporting constant operand in la will be much cleaner and readable than modifying the existing defintion to accept bare symbol and constant as argument.

+++ llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

+ OperandMatchResultTy parseBareSymbolOrConstant(OperandVector &Operands);

+ // Allow Constant Immediate as well as BareSymbols
+ bool isBareSymbolOrConstant() const {
+ if (isBareSymbol())
+ return true;
+ int64_t Imm;
+ RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
+ if (!isImm())
+ return false;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsInRange = isInt<32>(Imm) || isUInt<32>(Imm) || isRV64Imm();
+ return IsConstantImm && IsInRange && VK == RISCVMCExpr::VK_RISCV_None;
+ }
+

+ case Match_InvalidBareSymbolOrConstant: {
+ return generateImmOutOfRangeError(
+ Operands, ErrorInfo, std::numeric_limits<int32_t>::min(),
+ std::numeric_limits<uint32_t>::max(),
+ "operand either must be a Bare Symbol or an integer in the range");
+ }

+OperandMatchResultTy RISCVAsmParser::parseBareSymbolOrConstant(OperandVector &Operands) {
+ if (getLexer().getKind() == AsmToken::Identifier) {
+ OperandMatchResultTy Result = parseImmediate(Operands);
+ if (Result == MatchOperand_Success)
+ return MatchOperand_Success;
+ }
+ return parseBareSymbol(Operands);
+}
+

+++ llvm/lib/Target/RISCV/RISCVInstrInfo.td

+def BareSymbolOrConstant : AsmOperandClass {
+ let Name = "BareSymbolOrConstant";
+ let RenderMethod = "addImmOperands";
+ let DiagnosticType = "InvalidBareSymbolOrConstant";
+ let ParserMethod = "parseBareSymbolOrConstant";
+}
+

+// support constant operand for la pseudoinstruction.
+def bare_symbol_or_constant : Operand<XLenVT> {
+ let ParserMatchClass = BareSymbolOrConstant;
+}
+

-def PseudoLA : Pseudo<(outs GPR:$dst), (ins bare_symbol:$src), [],
+def PseudoLA : Pseudo<(outs GPR:$dst), (ins bare_symbol_or_constant:$src), [],

garvitgupta08 marked an inline comment as done.Jun 8 2023, 10:06 PM
garvitgupta08 marked an inline comment as not done.Jun 14 2023, 9:07 AM

Hi, I would like to merge this patch. Please let me know if new changes are fine. Thanks!

Hi @jrtc27, an update on the new changes will be appreciated!. Thanks

Hi @craig.topper , can you please review the patch and suggest if it needs any modifications/additions. Thanks!

Are we going to fix the same compatibility issue with "lla" and "lga"?

Are we going to fix the same compatibility issue with "lla" and "lga"?

I do not plan to work on it at the moment.

Are we going to fix the same compatibility issue with "lla" and "lga"?

I do not plan to work on it at the moment.

Would supporting that justify the approach using bare_symbol_or_constant?

Are we going to fix the same compatibility issue with "lla" and "lga"?

I do not plan to work on it at the moment.

Would supporting that justify the approach using bare_symbol_or_constant?

I have added support for lla as well. I believe that having a separate definition for immediate operand is much clean and readable than modifying the existing implementation (as shown in the diff above).

garvitgupta08 retitled this revision from [RISCV] Support constant immediate for la pseudo instruction to [RISCV] Support constant operand for la pnd lla pseudoinstruction..Jun 28 2023, 2:13 PM
garvitgupta08 edited the summary of this revision. (Show Details)
craig.topper accepted this revision.Jun 30 2023, 5:56 PM
craig.topper retitled this revision from [RISCV] Support constant operand for la pnd lla pseudoinstruction. to [RISCV] Support constant operand for la and lla pseudoinstruction..

LGTM

This revision is now accepted and ready to land.Jun 30 2023, 5:57 PM

LGTM

Can you please commit this patch on my behalf? I do not have commit access. Thanks!

Username - garvitgupta08
Name - Garvit Gupta
Mail - quic_garvgupt@quicinc.com

Fixing Premerge checks

llvm/lib/Target/RISCV/RISCVInstrInfo.td