Page MenuHomePhabricator

revert "[CodeGen] Require a name for a block addr target"
AcceptedPublic

Authored by nickdesaulniers on Jul 22 2022, 12:33 PM.

Details

Summary

Reverts commit 79176a2542d03107b90613c84f18ccba41ad8fa8.

After D130316, this is no longer necessary.

Diff Detail

Unit TestsFailed

TimeTest
80 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 22 2022, 12:33 PM
nickdesaulniers requested review of this revision.Jul 22 2022, 12:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2022, 12:33 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
216–217

Note: we already assert, so having a conditional later is pointless. I suspect the assert was added after 79176a2542d03107b90613c84f18ccba41ad8fa8 (but haven't checked the git log) and that this should have been fixed up when the assert was added.

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).

barannikov88 added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
232

The old behavior is to call createNamedTempSymbol
I can't judge if the new behavior is wrong or not, just noticed that it has changed.

nickdesaulniers marked an inline comment as done.

Bumping for review

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
232

Correct; I'm intentionally reverting that behavior.

The regression in llvm/test/CodeGen/RISCV/inline-asm-S-constraint.ll was due to a different patch in the series, https://reviews.llvm.org/D130323, which I've removed from the dependency chain for now.

nickdesaulniers marked an inline comment as done.Aug 17 2022, 12:36 PM

We still allow referring to a blockaddress in an asm operand, right? Does that still work with this patch? Something like the following:

long f() {
  void* z = &&X;
  goto *z;
  long a;
  X: asm("lea %1(%%rip), %0" : "=r"(a) : "i"(&&X));
  return a;
}

We still allow referring to a blockaddress in an asm operand, right? Does that still work with this patch? Something like the following:

long f() {
  void* z = &&X;
  goto *z;
  long a;
  X: asm("lea %1(%%rip), %0" : "=r"(a) : "i"(&&X));
  return a;
}

works just fine with this patch.

nickdesaulniers removed a subscriber: efriedma.
This revision is now accepted and ready to land.Aug 17 2022, 3:11 PM