[PATCH] D69887: [FEnv] File with properties of constrained intrinsics

Wed Nov 6 06:45:29 PST 2019

uweigand added a comment.

In D69887#1735367 <https://reviews.llvm.org/D69887#1735367>, @sepavloff wrote:

> In D69887#1735334 <https://reviews.llvm.org/D69887#1735334>, @uweigand wrote:
>
> > I really like that approach.  Unfortunately, this part of it does sort of partially revert one of the changes I recently made:
> >
> >   if (Opcode == ISD::STRICT_FP_ROUND)
> >     Opers.push_back(
> >         DAG.getTargetConstant(0, sdl, TLI.getPointerTy(DAG.getDataLayout())));
> >   
> >
> >
>
>
> Actually it was moved below in the same file to the line 6900.

What I meant was: originally, there were **two** IR opcode checks, a first check (switch statement) to determine the DAG opcode, and a second check (just one "if" now, but needs to become another switch with the comparison patch) to determine extra DAG operands.  My recent patch merged the two into a **single** opcode check (switch statement) -- and this patch again creates two separate checks.

>> The comparison patch (https://reviews.llvm.org/D69281) will introduce a lot more cases where we need an extra argument on the DAG side.
>> 
>> So maybe the best solution would be to extend the .def file with an additional column listing "extra arguments" for the DAG node -- then the DAG node could be constructed fully automatically without any per-opcode special cases in this routine.
> 
> It is not clear to me how to encode these "extra arguments". I would propose to limit auto generation to opcodes only, leaving attaching operands to custom code. There may be cases when building operands is complex enough and simple macro expansion implementation is not sufficient.

All currently known cases simply add one extra immediate integer argument.  I'm wondering whether it wouldn't make sense to address that common case automatically.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69887