[PATCH] D114895: [SelectionDagBuilder] improve CallBrInst BlockAddress constraint handling

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers added a comment.

Shower-epiphany: we just need to walk the arg_list in reverse until we find the first `blockaddress` input; the number of non-`blockaddress` inputs is the amount we must bias the index calculation to find the right block address.

Unless you can tie a block address. Hopefully `"+r"(&&goto_label)` isn't a valid output constraint...

  void x(void) {
      asm goto ("":"+r"(&&foo):::foo);
      //                ^ error: lvalue required in 'asm' statement
      foo:;
  }

oh thank god! ok then I can probably make that work.  Nonsensical rambling/notes below:

---

In D114895#3167718 <https://reviews.llvm.org/D114895#3167718>, @nickdesaulniers wrote:

> Or perhaps even just requiring that goto labels always were the final inputs. ie. example 3 would change to:
>
>   %1 = callbr i32 asm "# $0\0A\09# $1\0A\09# ${2:l}\0A\09", "={edi},rm,{edi},X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %3), i32 undef, i8* blockaddress(@foo, %3))
>             to label %2 [label %3]

While it is trivial to reorder the processing `labels` vs `InOutArgs` in `CodeGenFunction::EmitAsmStmt`, this isn't quite right; the positional format strings in the asm string would need to change (from `${2:l}` to `${3:l}`), something like:

  %1 = callbr i32 asm "# $0\0A\09# $1\0A\09# ${3:l}\0A\09", "={edi},rm,{edi},X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %3), i32 undef, i8* blockaddress(@foo, %3))
             to label %2 [label %3]

I'm not sure how I feel about renumbering the asm string from the user like that.  But I get the feeling that the "hidden" in-out args in the parameter list could be referred to in the asm string `0:-)`.

Finally, a more interesting ambiguity that perhaps should be addressed in the langref, specifically for the code in question here, consider:

  %1 = callbr i32 asm "# $0\0A\09# ${1:l}\0A\09", "={edi},X,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %3), i8* blockaddress(@foo, %3))
            to label %2 [label %3]

So basically we have 2 block addresses as input. Which `blockaddress` in the parameter list is the indirect target?  It does matter, specifically because of the asm string, the order matters for what the inline asm is trying to do. ie. I think we should be able to flip the order in IR and have that work (even if that's not the order clang will generate today), like:

  %1 = callbr i32 asm "# ${0:l}\0A\09# $1\0A\09", "={edi},X,X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@foo, %3), i8* blockaddress(@foo, %3))
            to label %2 [label %3]

The LLVM code in question here in `SelectionDAGBuilder::visitInlineAsm` (even with my proposed changes of walking the arg list backwards) //is// reliant on the unspecified assumption that the inputs are ordered in form "vanilla inputs, labels, hidden tied-inputs" because that's the order that Clang currently emits them in.  As demonstrated with this issue, that is somewhat brittle, but worse is both tightly coupled to the front-end/clang AND unspecified in the langref.  Theoretically, another frontend other than clang could try to make use of `callbr` and violate that assumption (see example IR immediately above).  I //hope// that never happens, but it's not impossible.  I still think my idea for computing the bias by walking the arg list backwards is the best approach today, but the reviewers should consider whether this ambiguity I'm highlighting actually exists, and if so, should we provide stronger language in the langref about this particularly in this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114895