[PATCH] D129288: [IR] Don't use blockaddresses as callbr arguments

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

While it makes me a bit sad to "lose" theoretical possibilities of the original design (e.g. I always thought it might be nice if someday we could use callbr to represent the potential control-transfer from a function call in a setjmp function back to the setjmp), I do think this is absolutely better for the ACTUAL current use-cases, and we'd've been better-off doing this from the get-go. Oops...my bad...

In D129288#3637110 <https://reviews.llvm.org/D129288#3637110>, @nickdesaulniers wrote:

> Does the langref mention any llvm-specific constraint string symbols?  Regardless, such a change should be documented very explicitly since it doesn't collide with any constraint string AFAIK today, but theoretically could in the future should the higher level languages implementing extended inline asm choose such a character as the constraint representation.

This isn't an issue. In fact, the syntax is entirely llvm-specific, and is separate from the frontend syntax parsed by Clang. It bears some resemblance to the GCC constraint syntax, but it is not the same.

Other comments:

- Clang won't emit them, but I'd like to see a test-case that uses different constraints for label args than just `!i`. E.g. `!r`, or `!0`. (Given the code structure, I expect it should just work, already.)



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5512
 
+      // Upgrade explicit blockaddress arguments to label constraints.
+      unsigned LabelNo = 0;
----------------
I'd put `if (isa<InlineAsm>(Callee))` around this whole upgrade section. While we only currently support inlineasm calee, the reader code is otherwise agnostic to the type of the callee, and it seems better to preserve that.

Also, this upgrade code seems potentially problematic -- a user could write the C code:
`int foo() { label: asm goto("" : : "r"(&&label) : : label2); return 1; label2: return 2; }`
which turns into
```
  callbr void asm sideeffect "", "r,i,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %2), i8* blockaddress(@foo, %4)) #1
          to label %3 [label %4]
```
and this would error out on it.

(I'm not sure _why_ a user would write that, but we shouldn't fail in the bitcode reader if they do!)

I think it needs to:
1. Only do anything if the constraint string doesn't contain label constraints (so we don't run this code on new bitcode).
2. Only validate and remove the last IndirectDests.size() arguments -- not other blockaddress values that may appear earlier in the arglist.

(And of course, a test-case. Are there any test-cases for this code? I didn't see one.)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8730
     if (OpInfo.hasArg()) {
-      OpInfo.CallOperandVal = Call.getArgOperand(ArgNo);
       OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
----------------
Why is this removed?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8758
+      auto *CB = cast<CallBrInst>(&Call);
+      OpInfo.CallOperandVal = CB->getBlockAddressForIndirectDest(LabelNo);
+      OpInfo.CallOperand = getValue(OpInfo.CallOperandVal);
----------------
I'm not sure we actually need to create a BlockAddress in SDAG either. But, fine to leave it for now, to keep the patch size down.


================
Comment at: llvm/lib/IR/InlineAsm.cpp:286
+    case InlineAsm::isLabel:
+      // We don't have access to labels here, will be verified separately.
+      break;
----------------
Should still check:
`if (NumClobbers) return false;               // inputs before clobbers.`



================
Comment at: llvm/lib/IR/Verifier.cpp:2232-2238
+  if (LabelNo) {
+    auto *CallBr = dyn_cast<CallBrInst>(&Call);
+    Check(CallBr, "Label constraints can only be used with callbr", &Call);
+    Check(LabelNo == CallBr->getNumIndirectDests(),
+          "Number of label constraints does not match number of callbr dests",
+          &Call);
+  }
----------------
Need to verify that there's zero indirect dests when there's no label constraints. Maybe like this?


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-errors.ll:1
-; RUN: not llc -mtriple=i686-- < %s 2> %t
-; RUN: FileCheck %s < %t
-
-; CHECK: Duplicate callbr destination
-
-; A test for asm-goto duplicate labels limitation
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=i686-- -verify-machineinstrs < %s | FileCheck %s
----------------
This is no longer an "error" test, as per filename. Maybe merge into the "callbr-asm-destinations.ll" file?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129288/new/

https://reviews.llvm.org/D129288