Page MenuHomePhabricator

revert "[MC] Don't recreate a label if it's already used"
Changes PlannedPublic

Authored by nickdesaulniers on Jul 21 2022, 6:17 PM.

Details

Summary

Reverts commit 1b104388752f66191c867380efde7bbf1f13ca80.

After D130316, this is no longer necessary.

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 debian > LLVM.CodeGen/RISCV::inline-asm-S-constraint.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=riscv32 < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll --check-prefix=RV32

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 6:17 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nickdesaulniers requested review of this revision.Jul 21 2022, 6:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 6:17 PM
arsenm accepted this revision.Jul 21 2022, 6:23 PM

LGTM. Feels wrong that inline asm would necessitate special label handling

This revision is now accepted and ready to land.Jul 21 2022, 6:23 PM
void added a comment.Jul 22 2022, 2:20 PM

LGTM. Feels wrong that inline asm would necessitate special label handling

"Inline asm feels wrong."

Fixed that for you. :-P

nickdesaulniers planned changes to this revision.Jul 22 2022, 3:47 PM

Turning on more build targets, either this or https://reviews.llvm.org/D130391 regresses (llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll).

nickdesaulniers accepted this revision.Jul 22 2022, 3:48 PM

Turning on more build targets, either this or https://reviews.llvm.org/D130391 regresses (llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll).

It's the child patch.

This revision is now accepted and ready to land.Aug 17 2022, 11:38 AM
nickdesaulniers planned changes to this revision.Aug 17 2022, 11:39 AM

It's the child patch.

i.e. this one regresses llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll. I'm going to rebase D130391 to go in first, since that shouldn't be controversial. Let's see.

I think I've found the issue: AddrLabelMap::getAddrLabelSymbolToEmit uses createNamedTempSymbol() when a BasicBlock has its address taken (i.e. blockaddress), but it doesn't update the symbol table (MCContext's Symbols member), so AsmParser can't find the existing symbol.

It looks like in the failing test case llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll that

createSymbol is called with .Ltmp to create the initial BB label, but then later getOrCreateSymbol calls createSymbol for .Ltmp0.

It seems like AddrLabelMap::getAddrLabelSymbolToEmit should have just added .Ltmp0 to MCContext's Symbols then and there.