[PATCH] D114959: Print target inline asm memory constraints

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/IR/InlineAsm.h:266
     Constraint_Zy,
     Constraints_Max = Constraint_Zy,
     Constraints_ShiftAmount = 16,
----------------
borisboesler wrote:
> kschwarz wrote:
> > As far as I understand, upstream targets define their (target) specific memory constraints in this common enum.
> > The Constraints_Max value is used in the assertion to make sure all upstream constraints are handled properly.
> > 
> > Why do you need to remove the assertion upstream? I would rather expect your downstream target to add more values to this enum, and to handle them in `getMemConstraintName`.
> Why should we add target related things in `llvm/IR`? We do not add target opcodes in `llvm/IR/ISDOpcodes.h`. I propose to keep target related code in their target codebase.
> As far as I understand, upstream targets define their (target) specific memory constraints in this common enum.

That's true at the moment but as far as I can remember there was no particularly good reason for it to be here rather than in the target (for example, AFAICT the numbers aren't used in the bitcode format). There just wasn't anything justifying the extra plumbing.

For in-tree targets there isn't really a practical reason to move it to the target but I agree it belongs there in principle. For out-of-tree targets there's some small but nice befits for both merge pain and binary compatibility (e.g. a shared library adding a new target to an existing LLVM)

LGTM


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

https://reviews.llvm.org/D114959