[PATCH] D114959: Print target inline asm memory constraints

[Boris Boesler via Phabricator via llvm-commits]

borisboesler marked an inline comment as not done.
borisboesler added inline comments.


================
Comment at: llvm/include/llvm/IR/InlineAsm.h:266
     Constraint_Zy,
     Constraints_Max = Constraint_Zy,
     Constraints_ShiftAmount = 16,
----------------
dsanders wrote:
> 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
> > 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

Thanks. I don't have commit access. Could you commit it for me, please?


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

https://reviews.llvm.org/D114959