Menu
▾
▴
avr-llvm-devel
|
From: Stepan D. <stp...@na...> - 2013-07-11 10:36:45
Attachments:
avr-inline-asm-mem-enh-2013-07-11.patch
|
Hello Borja,
> 1) you can reverse some conditions in the loop and use continues to
> reduce identation.
I've joined condition of three if-s, well it reduced code indentation,
though now we got single big if. Though I think its better to deal with
this 'if' instead of big indentations.
> 2) you can add this at the top of the loop
> if (ConstantSDNode *C =
dyn_cast<ConstantSDNode>(N->getOperand(i))) {
> Flag = C->getZExtValue();
> Kind = InlineAsm::getKind(Flag);
> }
> else
> continue;
If would possible if we have guarantee that all ConstantSDNode are
flags, but we haven't. So the problem is that before continue we should:
1. Skip all "sub-operands"
And straight after continue we should (or straight before real loop body):
2. Get flags for new operand
3. Extract NumVals (number of sub-operands or sub-values).
I have moved them into 'for':
2,3 => 'for' condition clause.
1. => 'for' evolution clause.
Finally I got next for pretty outstandard, so be ready :-) :
[code]
for (unsigned i = InlineAsm::Op_FirstOperand, Flags, NumVals;
(i != NumOps) &&
(Flags =
cast<ConstantSDNode>(Node->getOperand(i))->getZExtValue()) &&
(NumVals = InlineAsm::getNumOperandRegisters(Flags)) &&
(++i) /*Skip the ID value.*/;
i += NumVals
)
[/code]
> 3) watch out with placing the asterisk of pointers next to the variable
> not the type and i've seen some double spaces when passing arguments
> after the comma.
Fixed.
> 4) to be consistent with the rest of selectX functions please return a
> NULL when no matching is possible, build a new node when matching
> succeeds and break in the switch above to fall back to default behaviour
> when the result is NULL.
That was fixed too. Though on Select method we still have difference
with ARM implementation. Since they handle everything in SelectInlineAsm
method. Even operands processing.
While we have much less people in our team and have to keep default as
much as possible :-)
> 5) I guess SDLocs can be constructed once with the original node instead
> of constructing lots of differents ones.
I tried to keep same SDLocs as it was in nodes I replaced. Though for
add reg,imm opt I did as you asked.
> 6) I haven't checked this enough but, are we missing any glue operands
> when doing reg copies? Same for the glue operand of the inlneasm node,
> the ARM backend has them.
Glue is kind of dark horse to me. I asked Baldrick what it does. He says
that usually when instruction #1 changes some flag implicitly and
instruction #2 uses this flag (implicitly), you probably want to keep
these instructions "glued".
In inlineasm node I just passed all glue operands as-is without
changing. Do we need some extra glues for 'add reg, imm' opt?
-Stepan.
Borja Ferrer wrote:
> Hello Stepan,
>
> I like this approach better, it feels more general, and indeed doing it
> in ISel is a better place. The SelectAddr stuff it's fine, no need to
> move that new code you added into SelectInlineAsmMemoryOperand.
>
> Some comments about AVRDAGToDAGISel::SelectInlineAsm:
>
> 1) you can reverse some conditions in the loop and use continues to
> reduce identation.
> 2) you can add this at the top of the loop
> if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(N->getOperand(i))) {
> Flag = C->getZExtValue();
> Kind = InlineAsm::getKind(Flag);
> }
> else
> continue;
> to remove: for (; NumVals; --NumVals, ++i) when we need to continue the
> loop.
> I guess this is equivalent but please confirm.
> of course add a AsmNodeOperands.push_back(op); or whatever at the top of
> the loop to not ignore the operands that are skipped.
> 3) watch out with placing the asterisk of pointers next to the variable
> not the type and i've seen some double spaces when passing arguments
> after the comma.
> 4) to be consistent with the rest of selectX functions please return a
> NULL when no matching is possible, build a new node when matching
> succeeds and break in the switch above to fall back to default behaviour
> when the result is NULL.
> 5) I guess SDLocs can be constructed once with the original node instead
> of constructing lots of differents ones.
> 6) I haven't checked this enough but, are we missing any glue operands
> when doing reg copies? Same for the glue operand of the inlneasm node,
> the ARM backend has them.
>
>
>
> 2013/7/10 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> Hi Borja,
>
> > About the patch:
> > Is all the code in AVRISelLowering.cpp necesarry? I find it to be
> a very
> > "manual" way of doing this optimization, it feels like not being very
> > general, so please confirm me that this the only way of doing it. Im
> > asking this in case in AVRISelDAGToDAG.cpp you could add some code to
> > match this sort of DAGs with the existing adressing mode matching
> > functions to avoid the code in the other file. Maybe look at what
> other
> > backends do?
> I've compared it with ARM backend today (the only one that does
> something serious with InlineAsm node).
> Well all stuff I did in LowerINLINEASM is implemented in
> SelectInlineAsm, in ARMISelDAGToDAG.
> Well that's true, its not a lowering. I closer to the final stage,
> when you just correct the things and select proper instructions.
> So I did the same: I've moved everything from LowerINLINEASM to
> SelectInlineAsm. The new patch is attached.
>
> Theoretically, we can move everything related to memory constraint
> into single SelectInlineAsm method. But in this case I'll need to
> check all default behaviour that will be overridden. For me it is
> better to have as much default behaviour as possible.
>
> > In AVRISelDAGToDAG.cpp you've removed this line:
> > cast<MemSDNode>(Op)->__getMemoryVT().getSimpleVT();
> > to be:
> > + if (MemSDNode* MemNode = dyn_cast<MemSDNode>(Op))
> > Is this condition always going to be true? the cast above never
> failed
> > for me so in theory the dyn_cast should always be true, making
> the rest
> > of elses there always false.
> If Op is not a MemSDNode it dyn_cast will return false. So MemSDNode
> is load or store node. InlineAsm in our case could play the same
> role too. If you think it is some extra code in SelectAddr I can
> move it to SelectInlineAsmMemoryOperand, though I'll have to
> copypaste something from SelectAddr in this case.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Jej no problem, I know you've been busy with some ARM stuff.
>
> Yes let's get this in first, and then we can talk about the
> multibyte stuff.
>
> About the patch:
> Is all the code in AVRISelLowering.cpp necesarry? I find it to
> be a very
> "manual" way of doing this optimization, it feels like not being
> very
> general, so please confirm me that this the only way of doing it. Im
> asking this in case in AVRISelDAGToDAG.cpp you could add some
> code to
> match this sort of DAGs with the existing adressing mode matching
> functions to avoid the code in the other file. Maybe look at
> what other
> backends do?
>
> In AVRISelDAGToDAG.cpp you've removed this line:
> cast<MemSDNode>(Op)->__getMemoryVT().getSimpleVT();
> to be:
> + if (MemSDNode* MemNode = dyn_cast<MemSDNode>(Op))
> Is this condition always going to be true? the cast above never
> failed
> for me so in theory the dyn_cast should always be true, making
> the rest
> of elses there always false.
>
>
>
> 2013/7/9 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> Hello Borja,
> Sorry for latency, I had to be on dev. meeting.
> I've implemented optimization for memory constraint
> support. The
> patch is in attachment.
>
> P.S.: When I started to work on this opt, I forgot about
> multibyte
> constraints patch is not committed. So I can send it after
> we commit
> this opt.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Yes but the first goal now is to have inline asm
> feature complete xD
> Then we can move into other places of the library.
>
> Taking a look at the previous emails this is what needs
> still to
> be done:
> 1) Implement the missed optimization above for the memory
> constraint.
> Also see if you can come with further cases. Remember
> to add a
> test case.
> 2) Implement the stuff in the "C names used in
> assembler code"
> section
> of the avrlibc manual. You mentioned some issues here in a
> previous email.
> 3) Implement the a0, a1, etc... constraints.
>
>
>
> 2013/7/1 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
>
> Hi Borja.
> 1. OK. Will add it.
> 2.
>
> > movw r30, r24
> > adiw r30, 1
> Yup, we can replace it with "load r24, Z+1". I'll
> improve
> LowerINLINEASM a bit then.
>
> 3. If I'm got right, our main goal now is to get
> avr-libc
> compilable? Whould you tell me would else remained
> to do with
> inline-asm?
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> 1) Yes, an assert could do it. Fixup your
> patch to only
> allow A
> down to
> D constraints.
> 2) This code was compiled with -O3, cant you
> reproduce it?
>
>
> 2013/6/29 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> Ok more things, I've looked into the 64bit
> constraints and gcc
> doesnt seem to support them, for this code:
>
> unsigned long long delay(unsigned long
> long a,
> unsigned
> long long b)
>
> {
> uint8_t cnt;
> asm volatile (
> "add %A0, %A1" "\n\t"
> "add %B0, %B1" "\n\t"
> "add %C0, %C1" "\n\t"
> "add %D0, %D1" "\n\t"
> "add %E0, %E1" "\n\t"
> "add %F0, %F1" "\n\t"
> "add %G0, %G1" "\n\t"
> "add %H0, %H1" "\n\t"
> : "=r" (a)
> : "r" (b));
> return a;
> }
>
> gcc produces (ignoring frames):
> movw r18,r10
> movw r20,r12
> movw r22,r14
> movw r24,r16
> /* #APP */
> ; 266 "test.c" 1
> add r10, r18
> add r11, r19
> add r12, r20
> add r13, r21
> add r10, r18
> add r10, r18
> add r10, r18
> add r10, r18
>
> ; 0 "" 2
> /* #NOAPP */
> movw r18,r10
> movw r20,r12
> movw r22,r14
> movw r24,r16
>
> notice how the last 4 add instructions
> are wrong.
>
>
>
> 2013/6/29 Borja Ferrer
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> Ignore my previous email, it was my
> fault.
> Now, for the
> following code:
>
> uint8_t delay(unsigned char *p)
>
> {
> uint8_t cnt=8;
> asm volatile (
> "ld %0, %1" "\n\t"
> : "=r" (cnt)
> : "Q" (p[1]));
> return cnt;
> }
>
> we get:
> movw r30, r24
> adiw r30, 1
> ;APP
> ld r24, Z
>
> ;NO_APP
> ret
>
> Ideally, that adiw should be folded
> into the load.
>
>
>
> 2013/6/29 Borja Ferrer
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> Stepan I've been testing the memory
> constraint but im
> getting an assertion for the
> following code:
>
> uint8_t delay(unsigned char **p)
> {
> uint8_t cnt=8;
> asm volatile (
> "ld %0, %1" "\n\t"
> : "=r" (cnt)
> : "Q" (p[7]));
> return cnt;
> }
>
> What's wrong in here?
>
>
> 2013/6/29 Borja Ferrer
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> 1) Ok then, I will look into it.
> 2) Yes, in theory types should be
> legalized up
> to i16
> max, but for some reason i
> got that
> assertion
> triggered,
> i think it was with multibyte
> inline
> asm, so
> this has to
> be solved.
>
>
> 2013/6/28 Borja Ferrer
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> Hello Stepan, I will take
> a look
> to your patch
> later, but some questions:
>
> 1) does avr-gcc support
> 64bit values?
> 2) now that you're
> working with
> big data
> types, does
> it make sense to remove
> the assert I
> commented out
> about value types that are
> different to i8
> AND i16?
>
>
>
> 2013/6/28 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> Hi Borja.
> This is almost the
> final multibyte
> constraint
> patch. Tests are not
> created yet.
>
> Currently I have
> issue with 64 bit
> values. Even
> if I pass them as
> inline asm
> operands, llvm
> still truncates them
> to i32.
>
> What works:
>
> long a; // 32 bit
> void f() {
> asm("instr %A0 %B0
> %C0 %D0": :
> "r"(a));
> }
>
>
> What doesn't work:
>
> long long a; // 64 bit
> void f() {
> asm("instr %A0 %B0
> %E0": :
> "r"(a));
> }
>
> The last one fires
> assertion
> since llvm
> allocates registers
> for the
> first 32
> bits only.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Yes fine
>
> El jueves, 27 de
> junio de
> 2013, Stepan
> Dyatkovskiy escribió:
>
> OK. Will do.
> Currently
> there is a
> patch
> with memory
> constraint and draft
> multibyte
> constraint
> (supports
> only A
> and B). To be
> clean under
> multibyte
> constraints
> I mean
> something
> like this:
> asm
> volatile("mov
> __tmp_reg__,
> %A0" "\n\t"
>
> "mov
> %A0, %B0"
> "\n\t"
>
> "mov %B0,
> __tmp_reg__" "\n\t"
>
> : "=r"
> (value)
>
> : "0"
> (value)
> );
>
> -Stepan.
> Borja Ferrer
> wrote:
>
> btw, if
> this is
> now feature
> complete you can
> commit it.
>
>
>
> 2013/6/27 Borja
> Ferrer
>
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>
>
> No,
> that
> code is a
> personal
> modification of
> your examples.
> If you
> aren't
> getting the
> correct
> output from clang
> that's
> because you
>
> probably
> didn't apply
> a patch
> i commited last
> week for
> clang in
> our SVN.
> The
> assertion you're
> getting
> looks reasonable,
> it's an old
> friend of
>
> mine. The
> reason is
> that if Y
> is being reserved
> as the frame
>
> pointer, you
> cant have an
> instruction that
> uses 2
> registers
> when
>
> only Z is
> available. I
> wouldn't mind too
> much
> about it.
>
>
> Now, my 2nd
> question:
>
> Is
> there
> anything
> else that
> needs to be
> covered for
> the memory
>
> constraint?
>
>
>
> 2013/6/27 Stepan
> Dyatkovskiy
>
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
>
> This one
> looks good.
> Though ./llc
> emits "run out of
> registers
>
> allocation. I'll
> look at
> it. Is that from
> avr-libc?
>
> -Stepan.
>
> Borja
> Ferrer wrote:
>
>
> Yes,
> and then I
> replied the
> following:
>
>
> For
> this C code:
>
>
> char
> delay_count;
>
> char
> aaa;
>
> uint8_t
> delay(unsigned
> char p)
>
> {
>
> uint8_t
> cnt;
>
> asm
> volatile (
>
> "inst %0,
> %1" "\n\t"
>
> : "=Q"
> (delay_count)
>
> :
> "Q" (aaa));
>
> return cnt;
>
> }
>
>
> Clang produces:
>
>
> define i8
> @delay(i8
> %p) #0 {
>
> entry:
>
> tail call
> void asm
> sideeffect "inst $0,
> $1\0A\09",
>
> "=*Q,*Q"(i8*
>
> @delay_count, i8*
> @aaa) #2, !srcloc !4
>
> ret i8 undef
>
> }
>
>
> notice the *Q
> constraints, so
> is there
> anything
> wrong in
> there?
>
>
> Is there
> anything else
> that needs to be
> covered
> for the
>
> memory
> constraint?
>
>
>
> 2013/6/27 Stepan
> Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>__>
>
>
>
> Hi Borja,
>
>
> >
> Stepan, reply
> in this thread to
> my 2
>
> questions again
>
> if
> the email
>
> got lost.
>
>
> My prev.
> reply:
>
>
>
> > Ahh,
> I didn't
> know avr-gcc was
> buggy on this
>
> feature,
> that's probably
>
> > the
> reason for
> why the code i
> pasted in my
> previous
>
> email didn't
>
> work.
>
> ...
>
> >
> What's wrong
> with clang? I
> fixed clang
> inline asm
>
> support back on
>
> >
> friday, so is
> there anything
> else that is
> broken?
>
> It
> should emit
> '*Q' instead of 'Q'.
> That's why
> code
>
> from
> prev. email
>
> didn't
> work, just
> compare -emit-llvm -S
> output of
>
> avr-clang with
>
> other
> backends,
>
>
> > About the
> getPointerRegClass
> function, what
> is it
>
> used
> for? Also,
>
> please
>
> > move the
> implementation to
> the cpp file
> instead of
>
> leaving it in
>
> the .h.
>
> Currently, it is
> used in registers
> coelescing
> pass.
>
> While being
>
> registers
> inflating, llvm
> lookups
> all register
> uses. If
>
> it found
>
> that
> register is
> used by
> inline-asm as memory
> operand,
>
> it
> requests
>
> the
> pointer class
> with this method
> (I think
> the largest
>
> one). But
>
> may be
> in future
> this method will
> be used in
> more cases.
>
>
> avr-gcc
> has buggy
> implementation of
> memory
> constraint,
>
> that's why
>
> avr-libs
> doesn't
> use it at all. We
> have a
> chance to be
>
> first here
>
>
> -Stepan
>
>
> Borja
> Ferrer wrote:
>
>
> Stepan, reply
> in this thread to
> my 2
> questions
>
> again if the
>
> email got lost.
>
>
>
> 2013/6/27
> Borja Ferrer
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>__>__>
>
>
>
>
>
> No, your
> last email only
> says : "So,
> can I
>
> commit that
> memory
>
> constraint patch?"
>
>
>
> 2013/6/27
> Stepan Dyatkovskiy
>
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>__>__>
>
>
>
>
> Hm... Didn't
> you get my
> previous
> mail? If
>
> not,
> I think,
>
> I have
>
> to
> change my mail box.
>
>
> -Stepan
>
>
>
> -------- Original
> Message --------
>
> Subject: Re:
> [avr-llvm-devel] Inline
>
> assembly. Mostly
>
> just
> a stub.
>
> Date: Tue, 25
> Jun 2013
> 18:30:46 +0400
>
> From: Stepan
> Dyatkovskiy
>
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>
>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> <mailto:stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>>__>__>
>
> To:
> Borja Ferrer
>
> <bor...@gm... <mailto:bor...@gm...>
> <mailto:bor...@gm... <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>__>
>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>>__>
>
> <mailto:bor...@gm...
> <mailto:bor...@gm...> <mailto:bor...@gm...
> <mailto:bor...@gm...>>
> <mailto:bor...@gm...
> <mailto:bor...@gm...>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-15 16:52:47
|
Thanks!
I've been cleaning a bit the code, and fixed a possible crash here:
+ CanHandleRegImmOpt &= ImmNode != 0;+ CanHandleRegImmOpt &=
ImmNode->getAPIntValue().getZExtValue() < 64;
Since ImmNode comes from a dyncast, if it's NULL the second line would crash.
There's a piece of code i initially removed because i found it redundant:
+ if (ImmNode->getValueType(0) != MVT::i8)+ {+ Disp =
CurDAG->getTargetConstant(+
ImmNode->getAPIntValue().getZExtValue(), MVT::i8);+ }+ else+
{+ Disp = ImmOp;+ }
but without im getting crashes. Can you explain why this happens and is needed?
2013/7/15 Stepan Dyatkovskiy <stp...@na...>
> r269
> -Stepan
> Borja Ferrer wrote:
>
>> Ohhh this is a lot better, atleast fixing patches iteratively brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine verifier enabled then
>> feel free to commit it. I'll take a look at the glue operands after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't understand why I didn't
>> it in beginning.
>>
>> I moved everything into SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it implemented in hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to keep all chains in
>> proper order. But you rather look at new SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression tests, enable locally
>> the
>> > machine verifier to catch any additional errors. To enable it
>> > unconditionally search for the command object (i think it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on by default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD. You could split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite hard to read.
>>
>> 2) Yes I thought about that aswell, that not all constant nodes
>> are asm
>> flags, but the ARM backend does this, so either they have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition inside the for()
>> into the
>> top of the loop, it makes the for() quite unreadable. And adjust
>> there
>> the i variable as needed for each suboperand as you mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the SelectInlineAsm
>> method?
>> As far as i can see they only handle one specific case,
>> otherwise they
>> return NULL and let the default behaviour do the work.
>>
>> 6) Yes glues are a bit tricky, dont worry about them, I will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression tests, enable
>> locally the
>> machine verifier to catch any additional errors. To enable it
>> unconditionally search for the command object (i think it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on by default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-15 17:30:39
|
Hi Borja,
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().getZExtValue() < 64;
You absolutely right here. Thanks.
> There's a piece of code i initially removed because i found it redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
Initially did the same: just "Disp = ImmOp", and got the crash. I don't
remember what crash says exactly. Currently, I'm not able to compile
anything. What I had found out is that Disp (aka displacement) should be
i8 only. Our back-end just don't expect other types of displacements.
-Stepan.
Borja Ferrer wrote:
> Thanks!
>
> I've been cleaning a bit the code, and fixed a possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL the second line would crash.
>
>
>
> There's a piece of code i initially removed because i found it redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why this happens and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing patches iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the machine verifier enabled
> then
> feel free to commit it. I'll take a look at the glue operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
>
> Hello Borja.
> Seems I've made things much simpler. Can't understand why I
> didn't
> it in beginning.
>
> I moved everything into SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I just tried to keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the regression tests,
> enable locally the
> > machine verifier to catch any additional errors. To
> enable it
> > unconditionally search for the command object (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> > doing your own tests you can enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big conditions xD. You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if() that is quite hard to read.
>
> 2) Yes I thought about that aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does this, so either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the big condition inside the
> for()
> into the
> top of the loop, it makes the for() quite unreadable.
> And adjust
> there
> the i variable as needed for each suboperand as you
> mentioned
> before.
>
> 4) Why do you say they handle everything in the
> SelectInlineAsm
> method?
> As far as i can see they only handle one specific case,
> otherwise they
> return NULL and let the default behaviour do the work.
>
> 6) Yes glues are a bit tricky, dont worry about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the regression tests, enable
> locally the
> machine verifier to catch any additional errors. To
> enable it
> unconditionally search for the command object (i think it's
> declared in
> codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> doing your own tests you can enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-15 18:34:15
|
Just remembered after hitting resend that this has to be done always,
similarly like in SelectAddr(). However in this case it can be done
unconditionally since imm is guaranteed to fit in 6bits, so even if it
comes in a constant node of type i64 it's safe to truncate it to i8.
2013/7/15 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &= ImmNode->getAPIntValue().**getZExtValue() <
> 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because i found it redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > + ImmNode->getAPIntValue().**getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got the crash. I don't
> remember what crash says exactly. Currently, I'm not able to compile
> anything. What I had found out is that Disp (aka displacement) should be i8
> only. Our back-end just don't expect other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
>> Thanks!
>>
>> I've been cleaning a bit the code, and fixed a possible crash here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &= ImmNode->getAPIntValue().**getZExtValue() <
>> 64;
>>
>> Since ImmNode comes from a dyncast, if it's NULL the second line would
>> crash.
>>
>>
>>
>> There's a piece of code i initially removed because i found it redundant:
>> + if (ImmNode->getValueType(0) != MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> + ImmNode->getAPIntValue().**getZExtValue(), MVT::i8);
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you explain why this happens and is
>> needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better, atleast fixing patches iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine verifier enabled
>> then
>> feel free to commit it. I'll take a look at the glue operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>>
>> <mailto:stp...@na...>>>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it implemented in
>> hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to keep all
>> chains in
>> proper order. But you rather look at new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression tests,
>> enable locally the
>> > machine verifier to catch any additional errors. To
>> enable it
>> > unconditionally search for the command object (i think
>> it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on by
>> default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD. You could
>> split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite hard to read.
>>
>> 2) Yes I thought about that aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM backend does this, so either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition inside the
>> for()
>> into the
>> top of the loop, it makes the for() quite unreadable.
>> And adjust
>> there
>> the i variable as needed for each suboperand as you
>> mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they only handle one specific case,
>> otherwise they
>> return NULL and let the default behaviour do the work.
>>
>> 6) Yes glues are a bit tricky, dont worry about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression tests, enable
>> locally the
>> machine verifier to catch any additional errors. To
>> enable it
>> unconditionally search for the command object (i think
>> it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on by
>> default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-16 11:34:55
Attachments:
avr-inline-asm-multibyte-2013-07-16.patch
|
Back to multibyte referenced.
I've attached patch with this feature + unit-test. There is some
question though.
Currently, .c code:
int a;
void f() {
asm("instr %A0 %B0": : "r"(a));
}
is transformed into IR below:
define void @f(i16 %a) {
entry:
call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
ret void
}
To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
to return DREGS when it see i16 type or bigger. Though, may be you want
to fix clang and replace "r" constraint with another one.
-Stepan.
Borja Ferrer wrote:
> Just remembered after hitting resend that this has to be done always,
> similarly like in SelectAddr(). However in this case it can be done
> unconditionally since imm is guaranteed to fit in 6bits, so even if it
> comes in a constant node of type i64 it's safe to truncate it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...>>
>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().__getZExtValue() < 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because i found it
> redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > + ImmNode->getAPIntValue().__getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got the crash. I
> don't remember what crash says exactly. Currently, I'm not able to
> compile anything. What I had found out is that Disp (aka
> displacement) should be i8 only. Our back-end just don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed a possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().__getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL the second line
> would crash.
>
>
>
> There's a piece of code i initially removed because i found it
> redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> + ImmNode->getAPIntValue().__getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the machine
> verifier enabled
> then
> feel free to commit it. I'll take a look at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na... <mailto:stp...@na...>
>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
>
> Hello Borja.
> Seems I've made things much simpler. Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it
> implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I just tried to
> keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the regression tests,
> enable locally the
> > machine verifier to catch any additional errors. To
> enable it
> > unconditionally search for the command object
> (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> > doing your own tests you can enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big conditions xD.
> You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if() that is quite
> hard to read.
>
> 2) Yes I thought about that aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the big condition
> inside the
> for()
> into the
> top of the loop, it makes the for() quite
> unreadable.
> And adjust
> there
> the i variable as needed for each suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they handle everything in the
> SelectInlineAsm
> method?
> As far as i can see they only handle one
> specific case,
> otherwise they
> return NULL and let the default behaviour do
> the work.
>
> 6) Yes glues are a bit tricky, dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the regression
> tests, enable
> locally the
> machine verifier to catch any additional
> errors. To
> enable it
> unconditionally search for the command object
> (i think it's
> declared in
> codegen/passes.cpp as a cl::opt) and turn it on by
> default. When
> you're
> doing your own tests you can enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-16 13:09:32
|
What are you suggesting to do in clang?
2013/7/16 Stepan Dyatkovskiy <stp...@na...>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test. There is some question
> though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
> to return DREGS when it see i16 type or bigger. Though, may be you want to
> fix clang and replace "r" constraint with another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
>> Just remembered after hitting resend that this has to be done always,
>> similarly like in SelectAddr(). However in this case it can be done
>> unconditionally since imm is guaranteed to fit in 6bits, so even if it
>> comes in a constant node of type i64 it's safe to truncate it to i8.
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:stp...@na...
>> >>
>>
>> Hi Borja,
>>
>>
>> > + CanHandleRegImmOpt &= ImmNode != 0;
>> > + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>
>> You absolutely right here. Thanks.
>>
>>
>> > There's a piece of code i initially removed because i found it
>> redundant:
>> > + if (ImmNode->getValueType(0) != MVT::i8)
>> > + {
>> > + Disp = CurDAG->getTargetConstant(
>> > + ImmNode->getAPIntValue().__**getZExtValue(),
>> MVT::i8);
>> > + }
>> > + else
>> > + {
>> > + Disp = ImmOp;
>> > + }
>>
>> Initially did the same: just "Disp = ImmOp", and got the crash. I
>> don't remember what crash says exactly. Currently, I'm not able to
>> compile anything. What I had found out is that Disp (aka
>> displacement) should be i8 only. Our back-end just don't expect
>> other types of displacements.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>> Thanks!
>>
>> I've been cleaning a bit the code, and fixed a possible crash
>> here:
>>
>> + CanHandleRegImmOpt &= ImmNode != 0;
>> + CanHandleRegImmOpt &=
>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>
>> Since ImmNode comes from a dyncast, if it's NULL the second line
>> would crash.
>>
>>
>>
>> There's a piece of code i initially removed because i found it
>> redundant:
>> + if (ImmNode->getValueType(0) != MVT::i8)
>> + {
>> + Disp = CurDAG->getTargetConstant(
>> + ImmNode->getAPIntValue().__**getZExtValue(),
>> MVT::i8);
>> + }
>> + else
>> + {
>> + Disp = ImmOp;
>> + }
>>
>> but without im getting crashes. Can you explain why this happens
>> and is needed?
>>
>>
>>
>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...> <mailto:stp...@na...
>> <mailto:stp...@na...>>>
>>
>>
>> r269
>> -Stepan
>> Borja Ferrer wrote:
>>
>> Ohhh this is a lot better, atleast fixing patches
>> iteratively
>> brought us
>> to a nice solution :)
>>
>> If this patch passes all tests with the machine
>> verifier enabled
>> then
>> feel free to commit it. I'll take a look at the glue
>> operands
>> after this.
>>
>>
>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>
>> <mailto:stp...@na... <mailto:stp...@na...>>
>> <mailto:stp...@na... <mailto:stp...@na...>
>>
>> <mailto:stp...@na... <mailto:stp...@na...>>>>
>>
>>
>> Hello Borja.
>> Seems I've made things much simpler. Can't
>> understand why I
>> didn't
>> it in beginning.
>>
>> I moved everything into
>> SelectInlineAsmMemoryOperand hook.
>> #1: We don't need 'for' at all. Since it
>> implemented in
>> hook caller.
>> #2: This problem disappeared, see #1.
>> #4: The same.
>> #6: Still not sure about glues. I just tried to
>> keep all
>> chains in
>> proper order. But you rather look at new
>> SelectInlineAsmMemoryOperand.
>>
>>
>> > One last thing, when you run the regression
>> tests,
>> enable locally the
>> > machine verifier to catch any additional errors.
>> To
>> enable it
>> > unconditionally search for the command object
>> (i think it's
>> declared in
>> > codegen/passes.cpp as a cl::opt) and turn it on
>> by
>> default. When
>> you're
>> > doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> > something like that to llc.
>> OK.
>>
>> -Stepan.
>>
>>
>> Borja Ferrer wrote:
>>
>> Hello Stepan,
>>
>> 1) Wow, those are really big conditions xD.
>> You could
>> split them
>> into
>> single conditions and use continues:
>> if (cond 1) continue;
>> if (cond 2) continue;
>> etc...
>>
>> instead of having a huge if() that is quite
>> hard to read.
>>
>> 2) Yes I thought about that aswell, that not all
>> constant nodes
>> are asm
>> flags, but the ARM backend does this, so
>> either they
>> have a bug
>> there or
>> it's safe?
>> I prefer if you could move the big condition
>> inside the
>> for()
>> into the
>> top of the loop, it makes the for() quite
>> unreadable.
>> And adjust
>> there
>> the i variable as needed for each suboperand
>> as you
>> mentioned
>> before.
>>
>> 4) Why do you say they handle everything in the
>> SelectInlineAsm
>> method?
>> As far as i can see they only handle one
>> specific case,
>> otherwise they
>> return NULL and let the default behaviour do
>> the work.
>>
>> 6) Yes glues are a bit tricky, dont worry
>> about them, I
>> will fix
>> them
>> when this gets commited.
>>
>> One last thing, when you run the regression
>> tests, enable
>> locally the
>> machine verifier to catch any additional
>> errors. To
>> enable it
>> unconditionally search for the command object
>> (i think it's
>> declared in
>> codegen/passes.cpp as a cl::opt) and turn it on
>> by
>> default. When
>> you're
>> doing your own tests you can enable it passing
>> -verify-machineisntrs or
>> something like that to llc.
>>
>>
>>
>>
>>
>>
>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-16 13:18:40
|
Also, I guess doing only this for r is not enough, you can use multibytes
with other register constraints.
2013/7/16 Borja Ferrer <bor...@gm...>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...>
>
>> Back to multibyte referenced.
>>
>> I've attached patch with this feature + unit-test. There is some question
>> though.
>> Currently, .c code:
>> int a;
>> void f() {
>> asm("instr %A0 %B0": : "r"(a));
>> }
>> is transformed into IR below:
>> define void @f(i16 %a) {
>> entry:
>> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
>> ret void
>> }
>> To prevent the crash I had to fix getRegForInlineAsmConstraint, I set it
>> to return DREGS when it see i16 type or bigger. Though, may be you want to
>> fix clang and replace "r" constraint with another one.
>>
>> -Stepan.
>>
>> Borja Ferrer wrote:
>>
>>> Just remembered after hitting resend that this has to be done always,
>>> similarly like in SelectAddr(). However in this case it can be done
>>> unconditionally since imm is guaranteed to fit in 6bits, so even if it
>>> comes in a constant node of type i64 it's safe to truncate it to i8.
>>>
>>>
>>> 2013/7/15 Stepan Dyatkovskiy <stp...@na... <mailto:
>>> stp...@na...>>
>>>
>>> Hi Borja,
>>>
>>>
>>> > + CanHandleRegImmOpt &= ImmNode != 0;
>>> > + CanHandleRegImmOpt &=
>>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>>
>>> You absolutely right here. Thanks.
>>>
>>>
>>> > There's a piece of code i initially removed because i found it
>>> redundant:
>>> > + if (ImmNode->getValueType(0) != MVT::i8)
>>> > + {
>>> > + Disp = CurDAG->getTargetConstant(
>>> > + ImmNode->getAPIntValue().__**getZExtValue(),
>>> MVT::i8);
>>> > + }
>>> > + else
>>> > + {
>>> > + Disp = ImmOp;
>>> > + }
>>>
>>> Initially did the same: just "Disp = ImmOp", and got the crash. I
>>> don't remember what crash says exactly. Currently, I'm not able to
>>> compile anything. What I had found out is that Disp (aka
>>> displacement) should be i8 only. Our back-end just don't expect
>>> other types of displacements.
>>>
>>> -Stepan.
>>>
>>> Borja Ferrer wrote:
>>>
>>> Thanks!
>>>
>>> I've been cleaning a bit the code, and fixed a possible crash
>>> here:
>>>
>>> + CanHandleRegImmOpt &= ImmNode != 0;
>>> + CanHandleRegImmOpt &=
>>> ImmNode->getAPIntValue().__**getZExtValue() < 64;
>>>
>>> Since ImmNode comes from a dyncast, if it's NULL the second line
>>> would crash.
>>>
>>>
>>>
>>> There's a piece of code i initially removed because i found it
>>> redundant:
>>> + if (ImmNode->getValueType(0) != MVT::i8)
>>> + {
>>> + Disp = CurDAG->getTargetConstant(
>>> + ImmNode->getAPIntValue().__**getZExtValue(),
>>> MVT::i8);
>>> + }
>>> + else
>>> + {
>>> + Disp = ImmOp;
>>> + }
>>>
>>> but without im getting crashes. Can you explain why this happens
>>> and is needed?
>>>
>>>
>>>
>>> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...> <mailto:stp...@na...
>>> <mailto:stp...@na...>>>
>>>
>>>
>>> r269
>>> -Stepan
>>> Borja Ferrer wrote:
>>>
>>> Ohhh this is a lot better, atleast fixing patches
>>> iteratively
>>> brought us
>>> to a nice solution :)
>>>
>>> If this patch passes all tests with the machine
>>> verifier enabled
>>> then
>>> feel free to commit it. I'll take a look at the glue
>>> operands
>>> after this.
>>>
>>>
>>> 2013/7/12 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...>
>>> <mailto:stp...@na... <mailto:stp...@na...>>
>>> <mailto:stp...@na... <mailto:stp...@na...>
>>>
>>> <mailto:stp...@na... <mailto:stp...@na...>>>>
>>>
>>>
>>> Hello Borja.
>>> Seems I've made things much simpler. Can't
>>> understand why I
>>> didn't
>>> it in beginning.
>>>
>>> I moved everything into
>>> SelectInlineAsmMemoryOperand hook.
>>> #1: We don't need 'for' at all. Since it
>>> implemented in
>>> hook caller.
>>> #2: This problem disappeared, see #1.
>>> #4: The same.
>>> #6: Still not sure about glues. I just tried to
>>> keep all
>>> chains in
>>> proper order. But you rather look at new
>>> SelectInlineAsmMemoryOperand.
>>>
>>>
>>> > One last thing, when you run the regression
>>> tests,
>>> enable locally the
>>> > machine verifier to catch any additional
>>> errors. To
>>> enable it
>>> > unconditionally search for the command object
>>> (i think it's
>>> declared in
>>> > codegen/passes.cpp as a cl::opt) and turn it on
>>> by
>>> default. When
>>> you're
>>> > doing your own tests you can enable it passing
>>> -verify-machineisntrs or
>>> > something like that to llc.
>>> OK.
>>>
>>> -Stepan.
>>>
>>>
>>> Borja Ferrer wrote:
>>>
>>> Hello Stepan,
>>>
>>> 1) Wow, those are really big conditions xD.
>>> You could
>>> split them
>>> into
>>> single conditions and use continues:
>>> if (cond 1) continue;
>>> if (cond 2) continue;
>>> etc...
>>>
>>> instead of having a huge if() that is quite
>>> hard to read.
>>>
>>> 2) Yes I thought about that aswell, that not
>>> all
>>> constant nodes
>>> are asm
>>> flags, but the ARM backend does this, so
>>> either they
>>> have a bug
>>> there or
>>> it's safe?
>>> I prefer if you could move the big condition
>>> inside the
>>> for()
>>> into the
>>> top of the loop, it makes the for() quite
>>> unreadable.
>>> And adjust
>>> there
>>> the i variable as needed for each suboperand
>>> as you
>>> mentioned
>>> before.
>>>
>>> 4) Why do you say they handle everything in the
>>> SelectInlineAsm
>>> method?
>>> As far as i can see they only handle one
>>> specific case,
>>> otherwise they
>>> return NULL and let the default behaviour do
>>> the work.
>>>
>>> 6) Yes glues are a bit tricky, dont worry
>>> about them, I
>>> will fix
>>> them
>>> when this gets commited.
>>>
>>> One last thing, when you run the regression
>>> tests, enable
>>> locally the
>>> machine verifier to catch any additional
>>> errors. To
>>> enable it
>>> unconditionally search for the command object
>>> (i think it's
>>> declared in
>>> codegen/passes.cpp as a cl::opt) and turn it
>>> on by
>>> default. When
>>> you're
>>> doing your own tests you can enable it passing
>>> -verify-machineisntrs or
>>> something like that to llc.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
|
|
From: Stepan D. <stp...@na...> - 2013-07-17 12:13:55
|
Hi Borja.
It should work for all other constraints as well. I don't like idea of
fixing clang too. So we can just update getRegForInlineAsmConstraint method.
I'd tried to compile avr-libc. Currently clang doesn't want to compile
everything with -O0. While configure script tests everything with -O0.
Do you have any ideas how to fix -O0 ?
-Stepan.
Borja Ferrer wrote:
> Also, I guess doing only this for r is not enough, you can use
> multibytes with other register constraints.
>
>
> 2013/7/16 Borja Ferrer <bor...@gm...
> <mailto:bor...@gm...>>
>
> What are you suggesting to do in clang?
>
>
> 2013/7/16 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>>
>
> Back to multibyte referenced.
>
> I've attached patch with this feature + unit-test. There is some
> question though.
> Currently, .c code:
> int a;
> void f() {
> asm("instr %A0 %B0": : "r"(a));
> }
> is transformed into IR below:
> define void @f(i16 %a) {
> entry:
> call void asm sideeffect "instr ${0:A} ${0:B}", "r"(i16 %a)
> ret void
> }
> To prevent the crash I had to fix getRegForInlineAsmConstraint,
> I set it to return DREGS when it see i16 type or bigger. Though,
> may be you want to fix clang and replace "r" constraint with
> another one.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Just remembered after hitting resend that this has to be
> done always,
> similarly like in SelectAddr(). However in this case it can
> be done
> unconditionally since imm is guaranteed to fit in 6bits, so
> even if it
> comes in a constant node of type i64 it's safe to truncate
> it to i8.
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
>
> Hi Borja,
>
>
> > + CanHandleRegImmOpt &= ImmNode != 0;
> > + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().____getZExtValue() < 64;
>
> You absolutely right here. Thanks.
>
>
> > There's a piece of code i initially removed because
> i found it
> redundant:
> > + if (ImmNode->getValueType(0) != MVT::i8)
> > + {
> > + Disp = CurDAG->getTargetConstant(
> > +
> ImmNode->getAPIntValue().____getZExtValue(), MVT::i8);
> > + }
> > + else
> > + {
> > + Disp = ImmOp;
> > + }
>
> Initially did the same: just "Disp = ImmOp", and got
> the crash. I
> don't remember what crash says exactly. Currently, I'm
> not able to
> compile anything. What I had found out is that Disp (aka
> displacement) should be i8 only. Our back-end just
> don't expect
> other types of displacements.
>
> -Stepan.
>
> Borja Ferrer wrote:
>
> Thanks!
>
> I've been cleaning a bit the code, and fixed a
> possible crash here:
>
> + CanHandleRegImmOpt &= ImmNode != 0;
> + CanHandleRegImmOpt &=
> ImmNode->getAPIntValue().____getZExtValue() < 64;
>
> Since ImmNode comes from a dyncast, if it's NULL
> the second line
> would crash.
>
>
>
> There's a piece of code i initially removed because
> i found it
> redundant:
> + if (ImmNode->getValueType(0) != MVT::i8)
> + {
> + Disp = CurDAG->getTargetConstant(
> +
> ImmNode->getAPIntValue().____getZExtValue(), MVT::i8);
> + }
> + else
> + {
> + Disp = ImmOp;
> + }
>
> but without im getting crashes. Can you explain why
> this happens
> and is needed?
>
>
>
> 2013/7/15 Stepan Dyatkovskiy <stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na...
> <mailto:stp...@na...>> <mailto:stp...@na...
> <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>>>
>
>
> r269
> -Stepan
> Borja Ferrer wrote:
>
> Ohhh this is a lot better, atleast fixing
> patches
> iteratively
> brought us
> to a nice solution :)
>
> If this patch passes all tests with the
> machine
> verifier enabled
> then
> feel free to commit it. I'll take a look
> at the glue
> operands
> after this.
>
>
> 2013/7/12 Stepan Dyatkovskiy
> <stp...@na... <mailto:stp...@na...>
> <mailto:stp...@na... <mailto:stp...@na...>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>
>
> <mailto:stp...@na...
> <mailto:stp...@na...> <mailto:stp...@na...
> <mailto:stp...@na...>>>>>
>
>
> Hello Borja.
> Seems I've made things much simpler.
> Can't
> understand why I
> didn't
> it in beginning.
>
> I moved everything into
> SelectInlineAsmMemoryOperand hook.
> #1: We don't need 'for' at all. Since it
> implemented in
> hook caller.
> #2: This problem disappeared, see #1.
> #4: The same.
> #6: Still not sure about glues. I
> just tried to
> keep all
> chains in
> proper order. But you rather look at new
> SelectInlineAsmMemoryOperand.
>
>
> > One last thing, when you run the
> regression tests,
> enable locally the
> > machine verifier to catch any
> additional errors. To
> enable it
> > unconditionally search for the
> command object
> (i think it's
> declared in
> > codegen/passes.cpp as a cl::opt)
> and turn it on by
> default. When
> you're
> > doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> > something like that to llc.
> OK.
>
> -Stepan.
>
>
> Borja Ferrer wrote:
>
> Hello Stepan,
>
> 1) Wow, those are really big
> conditions xD.
> You could
> split them
> into
> single conditions and use continues:
> if (cond 1) continue;
> if (cond 2) continue;
> etc...
>
> instead of having a huge if()
> that is quite
> hard to read.
>
> 2) Yes I thought about that
> aswell, that not all
> constant nodes
> are asm
> flags, but the ARM backend does
> this, so
> either they
> have a bug
> there or
> it's safe?
> I prefer if you could move the
> big condition
> inside the
> for()
> into the
> top of the loop, it makes the
> for() quite
> unreadable.
> And adjust
> there
> the i variable as needed for each
> suboperand
> as you
> mentioned
> before.
>
> 4) Why do you say they handle
> everything in the
> SelectInlineAsm
> method?
> As far as i can see they only
> handle one
> specific case,
> otherwise they
> return NULL and let the default
> behaviour do
> the work.
>
> 6) Yes glues are a bit tricky,
> dont worry
> about them, I
> will fix
> them
> when this gets commited.
>
> One last thing, when you run the
> regression
> tests, enable
> locally the
> machine verifier to catch any
> additional
> errors. To
> enable it
> unconditionally search for the
> command object
> (i think it's
> declared in
> codegen/passes.cpp as a cl::opt)
> and turn it on by
> default. When
> you're
> doing your own tests you can
> enable it passing
> -verify-machineisntrs or
> something like that to llc.
>
>
>
>
>
>
>
>
>
>
|
|
From: Borja F. <bor...@gm...> - 2013-07-23 19:04:38
|
I've been adding fixes to pass a ton of regression in the Generic folder. Currently there are two tests that fail with a "cannot select build_pair" related to adding 16bit reg classes in the inline asm code. The rest of failing tests are related with not being able to run llc with -O0. There's a serious codegen bug that i spotted yesterday while doing the fixes that I need to work on. 2013/7/22 Borja Ferrer <bor...@gm...> > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-libc/user-manual/inline_asm.html > but in case there are any doubts basically it works the same way as > multibyte constraints except that when you see an "aN", where N is a > number, you should print the alternative regname. Of course this should > only work for X Y and Z, not sure how gcc complains when you use another > register, or maybe it ignores it and prints the normal register name. > > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 07:21:27
|
Hello Borja, Great. I have found out next inline-asm issue. When you pass i32 bit type, it could be split onto two registers only, no matter which register class you set for it. This behaviour is implemented in SelectionDAGBuilder::visitInlineAsm. I think I'll implement simplest version of ExpandInlineAsm today, that will fix operand types properly. -Stepan Borja Ferrer wrote: > I've been adding fixes to pass a ton of regression in the Generic folder. > Currently there are two tests that fail with a "cannot select > build_pair" related to adding 16bit reg classes in the inline asm code. > The rest of failing tests are related with not being able to run llc > with -O0. There's a serious codegen bug that i spotted yesterday while > doing the fixes that I need to work on. > > > 2013/7/22 Borja Ferrer <bor...@gm... > <mailto:bor...@gm...>> > > Hello Stepan, > > 3) Yes, it's the second case. It's explained in > http://www.nongnu.org/avr-libc/user-manual/inline_asm.html > but in case there are any doubts basically it works the same way as > multibyte constraints except that when you see an "aN", where N is a > number, you should print the alternative regname. Of course this > should only work for X Y and Z, not sure how gcc complains when you > use another register, or maybe it ignores it and prints the normal > register name. > > |
|
From: Stepan D. <stp...@na...> - 2013-07-24 13:09:02
Attachments:
avr-inline-asm-multibyte-2013-07-24.patch
|
Hello Borja, Here is the new patch for multibyte reference. Please look at new tests to see what was implemented exactly. To fix issue I described in previous letter I've added new reg classes that contains pairs of 8 bit registers: If we pass i32 parameter that should be split onto four 8 bit regs, we set i16 register class first, then it would be replaced with 8 bit registers if needed. Currently I see it is the only solution that doesn't touch vmcore. -Stepan. Stepan Dyatkovskiy wrote: > Hello Borja, > Great. I have found out next inline-asm issue. > When you pass i32 bit type, it could be split onto two registers only, > no matter which register class you set for it. This behaviour is > implemented in SelectionDAGBuilder::visitInlineAsm. I think I'll > implement simplest version of ExpandInlineAsm today, that will fix > operand types properly. > > -Stepan > > Borja Ferrer wrote: >> I've been adding fixes to pass a ton of regression in the Generic folder. >> Currently there are two tests that fail with a "cannot select >> build_pair" related to adding 16bit reg classes in the inline asm code. >> The rest of failing tests are related with not being able to run llc >> with -O0. There's a serious codegen bug that i spotted yesterday while >> doing the fixes that I need to work on. >> >> >> 2013/7/22 Borja Ferrer <bor...@gm... >> <mailto:bor...@gm...>> >> >> Hello Stepan, >> >> 3) Yes, it's the second case. It's explained in >> http://www.nongnu.org/avr-libc/user-manual/inline_asm.html >> but in case there are any doubts basically it works the same way as >> multibyte constraints except that when you see an "aN", where N is a >> number, you should print the alternative regname. Of course this >> should only work for X Y and Z, not sure how gcc complains when you >> use another register, or maybe it ignores it and prints the normal >> register name. >> >> > > > ------------------------------------------------------------------------------ > See everything from the browser to the database with AppDynamics > Get end-to-end visibility with application monitoring from AppDynamics > Isolate bottlenecks and diagnose root cause in seconds. > Start your free trial of AppDynamics Pro today! > http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk > _______________________________________________ > avr-llvm-devel mailing list > avr...@li... > https://lists.sourceforge.net/lists/listinfo/avr-llvm-devel > |
|
From: Stepan D. <stp...@na...> - 2013-06-25 14:33:53
|
And... I mentioned Rick Mann, not John, since Rick said he would like to
help us with frontend. My fault.
-Stepan.
Stepan Dyatkovskiy wrote:
> Hi Borja,
>
> > Ahh, I didn't know avr-gcc was buggy on this feature, that's probably
>> the reason for why the code i pasted in my previous email didn't work.
> ...
>> What's wrong with clang? I fixed clang inline asm support back on
>> friday, so is there anything else that is broken?
> It should emit '*Q' instead of 'Q'. That's why code from prev. email
> didn't work, just compare -emit-llvm -S output of avr-clang with other
> backends,
>
> > About the getPointerRegClass function, what is it used for? Also, please
> > move the implementation to the cpp file instead of leaving it in the .h.
> Currently, it is used in registers coelescing pass. While being
> registers inflating, llvm lookups all register uses. If it found that
> register is used by inline-asm as memory operand, it requests the
> pointer class with this method (I think the largest one). But may be in
> future this method will be used in more cases.
>
> avr-gcc has buggy implementation of memory constraint, that's why
> avr-libs doesn't use it at all. We have a chance to be first here :-)
>
> -Stepan
>
>>
>>
>>
>> 2013/6/25 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>>
>>
>> Hi Borja.
>> This is new patch. Seems I've moved memory constraint implementation
>> to the same level like all other constraints. Note avr-gcc has buggy
>> implementation of memory constraint, that's why *avr-libc doesn't
>> use it*.
>>
>> I removed restriction for Y,Z in getLargestLegalSuperClass, so these
>> registers could be inflated now. But there was unimplemented
>> getPointerRegClass method in AVRRegistersInfo. So currently I have
>> restricted it to Y and Z. Though suppose we can extend it in future
>> to XYZ set.
>> Look changes in inline-asm.ll to see what exactly is supported now.
>>
>> About clang.
>> May be we ask John Myers to fix clang support for inline asm?
>>
>> -Stepan.
>>
>>
>> Stepan Dyatkovskiy wrote:
>>
>> The patch. Forgot to attach it.
>> -Stepan.
>> Stepan Dyatkovskiy wrote:
>>
>> Hi Borja,
>>
>> > What happens in your example above when you use a real
>> instruction
>> like
>>
>> for example LDD? Does it still use GPR8 regs? To me it's
>> weird that if
>> you use a real instruction where operands are clearly
>> defined in the td
>> file the instruction selector uses an invalid regclass.
>>
>>
>> There are two kinds of inline asm support in LLVM:
>> 1. You just support all the constraints and pastes
>> inline-asm contents
>> as-is. That's why its still allowed to use names like
>> "some_instr".
>> 2. You may expand inline-asm strings set, or in another
>> words just parse
>> it onto set of instructions. In this case you have to
>> implement
>> TargetLowering::__ExpandInlineAsm method.
>> I'd want to start it on this week though...
>>
>> But first we have to get avr-libc compilable (perhpas I
>> read you
>> thoughts ;-) )
>>
>> Relative to memory constrains. I implemented initial version
>> it can
>> catch simplest cases (from test-case):
>>
>> @a = internal global i16 0, align 4
>> @b = internal global i16 0, align 4
>> define void @mem() {
>> ;CHECK: some_instr Z, Y
>> call void asm "some_instr $0, $1", "=*Q,=*Q"(i16* @a,
>> i16* @b)
>> ret void
>> }
>>
>> The patch is attached.
>>
>> Its in my todo yet, to handle local variables. They could be
>> emitted as
>> Y+q expression. Hope to present this support tomorrow. So if
>> 'a' and 'b'
>> from example above would be local we could get "some_instr
>> Y, Y+2"
>>
>> -Stepan.
>>
>>
>>
>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2013-06-25 14:51:20
|
For this C code:
char delay_count;
char aaa;
uint8_t delay(unsigned char p)
{
uint8_t cnt;
asm volatile (
"inst %0, %1" "\n\t"
: "=Q" (delay_count)
: "Q" (aaa));
return cnt;
}
Clang produces:
define i8 @delay(i8 %p) #0 {
entry:
tail call void asm sideeffect "inst $0, $1\0A\09", "=*Q,*Q"(i8*
@delay_count, i8* @aaa) #2, !srcloc !4
ret i8 undef
}
notice the *Q constraints, so is there anything wrong in there?
Is there anything else that needs to be covered for the memory constraint?
2013/6/25 Stepan Dyatkovskiy <stp...@na...>
> And... I mentioned Rick Mann, not John, since Rick said he would like to
> help us with frontend. My fault.
>
>
> -Stepan.
>
> Stepan Dyatkovskiy wrote:
>
>> Hi Borja,
>>
>> > Ahh, I didn't know avr-gcc was buggy on this feature, that's probably
>>
>>> the reason for why the code i pasted in my previous email didn't work.
>>>
>> ...
>>
>>> What's wrong with clang? I fixed clang inline asm support back on
>>> friday, so is there anything else that is broken?
>>>
>> It should emit '*Q' instead of 'Q'. That's why code from prev. email
>> didn't work, just compare -emit-llvm -S output of avr-clang with other
>> backends,
>>
>> > About the getPointerRegClass function, what is it used for? Also,
>> please
>> > move the implementation to the cpp file instead of leaving it in the
>> .h.
>> Currently, it is used in registers coelescing pass. While being
>> registers inflating, llvm lookups all register uses. If it found that
>> register is used by inline-asm as memory operand, it requests the
>> pointer class with this method (I think the largest one). But may be in
>> future this method will be used in more cases.
>>
>> avr-gcc has buggy implementation of memory constraint, that's why
>> avr-libs doesn't use it at all. We have a chance to be first here :-)
>>
>> -Stepan
>>
>>
>>>
>>>
>>> 2013/6/25 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...>>
>>>
>>> Hi Borja.
>>> This is new patch. Seems I've moved memory constraint implementation
>>> to the same level like all other constraints. Note avr-gcc has buggy
>>> implementation of memory constraint, that's why *avr-libc doesn't
>>> use it*.
>>>
>>> I removed restriction for Y,Z in getLargestLegalSuperClass, so these
>>> registers could be inflated now. But there was unimplemented
>>> getPointerRegClass method in AVRRegistersInfo. So currently I have
>>> restricted it to Y and Z. Though suppose we can extend it in future
>>> to XYZ set.
>>> Look changes in inline-asm.ll to see what exactly is supported now.
>>>
>>> About clang.
>>> May be we ask John Myers to fix clang support for inline asm?
>>>
>>> -Stepan.
>>>
>>>
>>> Stepan Dyatkovskiy wrote:
>>>
>>> The patch. Forgot to attach it.
>>> -Stepan.
>>> Stepan Dyatkovskiy wrote:
>>>
>>> Hi Borja,
>>>
>>> > What happens in your example above when you use a real
>>> instruction
>>> like
>>>
>>> for example LDD? Does it still use GPR8 regs? To me it's
>>> weird that if
>>> you use a real instruction where operands are clearly
>>> defined in the td
>>> file the instruction selector uses an invalid regclass.
>>>
>>>
>>> There are two kinds of inline asm support in LLVM:
>>> 1. You just support all the constraints and pastes
>>> inline-asm contents
>>> as-is. That's why its still allowed to use names like
>>> "some_instr".
>>> 2. You may expand inline-asm strings set, or in another
>>> words just parse
>>> it onto set of instructions. In this case you have to
>>> implement
>>> TargetLowering::__**ExpandInlineAsm method.
>>> I'd want to start it on this week though...
>>>
>>> But first we have to get avr-libc compilable (perhpas I
>>> read you
>>> thoughts ;-) )
>>>
>>> Relative to memory constrains. I implemented initial version
>>> it can
>>> catch simplest cases (from test-case):
>>>
>>> @a = internal global i16 0, align 4
>>> @b = internal global i16 0, align 4
>>> define void @mem() {
>>> ;CHECK: some_instr Z, Y
>>> call void asm "some_instr $0, $1", "=*Q,=*Q"(i16* @a,
>>> i16* @b)
>>> ret void
>>> }
>>>
>>> The patch is attached.
>>>
>>> Its in my todo yet, to handle local variables. They could be
>>> emitted as
>>> Y+q expression. Hope to present this support tomorrow. So if
>>> 'a' and 'b'
>>> from example above would be local we could get "some_instr
>>> Y, Y+2"
>>>
>>> -Stepan.
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
|
|
From: Rick M. <rm...@la...> - 2013-06-25 19:44:18
|
On Jun 25, 2013, at 07:33 , Stepan Dyatkovskiy <stp...@na...> wrote: > And... I mentioned Rick Mann, not John, since Rick said he would like to help us with frontend. My fault. Hi, what? I'm sorry, I think I missed an email. As much as I'd like to work on this, I'm so incredibly swamped at work (Jul 8 deadline followed by intensive QA period), I'm afraid I won't be much use. Plus, you guys are clearly much more capable with LLVM than I am. You're making incredible progress. -- Rick |
|
From: Stepan D. <stp...@na...> - 2013-06-27 08:09:47
|
Hi Borja,
So, can I commit that memory constraint patch?
-Stepan.
Stepan Dyatkovskiy wrote:
> Hi Borja,
>
> > Ahh, I didn't know avr-gcc was buggy on this feature, that's probably
>> the reason for why the code i pasted in my previous email didn't work.
> ...
>> What's wrong with clang? I fixed clang inline asm support back on
>> friday, so is there anything else that is broken?
> It should emit '*Q' instead of 'Q'. That's why code from prev. email
> didn't work, just compare -emit-llvm -S output of avr-clang with other
> backends,
>
> > About the getPointerRegClass function, what is it used for? Also, please
> > move the implementation to the cpp file instead of leaving it in the .h.
> Currently, it is used in registers coelescing pass. While being
> registers inflating, llvm lookups all register uses. If it found that
> register is used by inline-asm as memory operand, it requests the
> pointer class with this method (I think the largest one). But may be in
> future this method will be used in more cases.
>
> avr-gcc has buggy implementation of memory constraint, that's why
> avr-libs doesn't use it at all. We have a chance to be first here :-)
>
> -Stepan
>
>>
>>
>>
>> 2013/6/25 Stepan Dyatkovskiy <stp...@na...
>> <mailto:stp...@na...>>
>>
>> Hi Borja.
>> This is new patch. Seems I've moved memory constraint implementation
>> to the same level like all other constraints. Note avr-gcc has buggy
>> implementation of memory constraint, that's why *avr-libc doesn't
>> use it*.
>>
>> I removed restriction for Y,Z in getLargestLegalSuperClass, so these
>> registers could be inflated now. But there was unimplemented
>> getPointerRegClass method in AVRRegistersInfo. So currently I have
>> restricted it to Y and Z. Though suppose we can extend it in future
>> to XYZ set.
>> Look changes in inline-asm.ll to see what exactly is supported now.
>>
>> About clang.
>> May be we ask John Myers to fix clang support for inline asm?
>>
>> -Stepan.
>>
>>
>> Stepan Dyatkovskiy wrote:
>>
>> The patch. Forgot to attach it.
>> -Stepan.
>> Stepan Dyatkovskiy wrote:
>>
>> Hi Borja,
>>
>> > What happens in your example above when you use a real
>> instruction
>> like
>>
>> for example LDD? Does it still use GPR8 regs? To me it's
>> weird that if
>> you use a real instruction where operands are clearly
>> defined in the td
>> file the instruction selector uses an invalid regclass.
>>
>>
>> There are two kinds of inline asm support in LLVM:
>> 1. You just support all the constraints and pastes
>> inline-asm contents
>> as-is. That's why its still allowed to use names like
>> "some_instr".
>> 2. You may expand inline-asm strings set, or in another
>> words just parse
>> it onto set of instructions. In this case you have to
>> implement
>> TargetLowering::__ExpandInlineAsm method.
>> I'd want to start it on this week though...
>>
>> But first we have to get avr-libc compilable (perhpas I
>> read you
>> thoughts ;-) )
>>
>> Relative to memory constrains. I implemented initial version
>> it can
>> catch simplest cases (from test-case):
>>
>> @a = internal global i16 0, align 4
>> @b = internal global i16 0, align 4
>> define void @mem() {
>> ;CHECK: some_instr Z, Y
>> call void asm "some_instr $0, $1", "=*Q,=*Q"(i16* @a,
>> i16* @b)
>> ret void
>> }
>>
>> The patch is attached.
>>
>> Its in my todo yet, to handle local variables. They could be
>> emitted as
>> Y+q expression. Hope to present this support tomorrow. So if
>> 'a' and 'b'
>> from example above would be local we could get "some_instr
>> Y, Y+2"
>>
>> -Stepan.
>>
>>
>>
>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2013-06-27 11:34:26
|
When you answer the 2 questions in my previous email :)
2013/6/27 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja,
> So, can I commit that memory constraint patch?
>
>
> -Stepan.
>
> Stepan Dyatkovskiy wrote:
>
>> Hi Borja,
>>
>> > Ahh, I didn't know avr-gcc was buggy on this feature, that's probably
>>
>>> the reason for why the code i pasted in my previous email didn't work.
>>>
>> ...
>>
>>> What's wrong with clang? I fixed clang inline asm support back on
>>> friday, so is there anything else that is broken?
>>>
>> It should emit '*Q' instead of 'Q'. That's why code from prev. email
>> didn't work, just compare -emit-llvm -S output of avr-clang with other
>> backends,
>>
>> > About the getPointerRegClass function, what is it used for? Also,
>> please
>> > move the implementation to the cpp file instead of leaving it in the
>> .h.
>> Currently, it is used in registers coelescing pass. While being
>> registers inflating, llvm lookups all register uses. If it found that
>> register is used by inline-asm as memory operand, it requests the
>> pointer class with this method (I think the largest one). But may be in
>> future this method will be used in more cases.
>>
>> avr-gcc has buggy implementation of memory constraint, that's why
>> avr-libs doesn't use it at all. We have a chance to be first here :-)
>>
>> -Stepan
>>
>>
>>>
>>>
>>> 2013/6/25 Stepan Dyatkovskiy <stp...@na...
>>> <mailto:stp...@na...>>
>>>
>>> Hi Borja.
>>> This is new patch. Seems I've moved memory constraint implementation
>>> to the same level like all other constraints. Note avr-gcc has buggy
>>> implementation of memory constraint, that's why *avr-libc doesn't
>>> use it*.
>>>
>>> I removed restriction for Y,Z in getLargestLegalSuperClass, so these
>>> registers could be inflated now. But there was unimplemented
>>> getPointerRegClass method in AVRRegistersInfo. So currently I have
>>> restricted it to Y and Z. Though suppose we can extend it in future
>>> to XYZ set.
>>> Look changes in inline-asm.ll to see what exactly is supported now.
>>>
>>> About clang.
>>> May be we ask John Myers to fix clang support for inline asm?
>>>
>>> -Stepan.
>>>
>>>
>>> Stepan Dyatkovskiy wrote:
>>>
>>> The patch. Forgot to attach it.
>>> -Stepan.
>>> Stepan Dyatkovskiy wrote:
>>>
>>> Hi Borja,
>>>
>>> > What happens in your example above when you use a real
>>> instruction
>>> like
>>>
>>> for example LDD? Does it still use GPR8 regs? To me it's
>>> weird that if
>>> you use a real instruction where operands are clearly
>>> defined in the td
>>> file the instruction selector uses an invalid regclass.
>>>
>>>
>>> There are two kinds of inline asm support in LLVM:
>>> 1. You just support all the constraints and pastes
>>> inline-asm contents
>>> as-is. That's why its still allowed to use names like
>>> "some_instr".
>>> 2. You may expand inline-asm strings set, or in another
>>> words just parse
>>> it onto set of instructions. In this case you have to
>>> implement
>>> TargetLowering::__**ExpandInlineAsm method.
>>> I'd want to start it on this week though...
>>>
>>> But first we have to get avr-libc compilable (perhpas I
>>> read you
>>> thoughts ;-) )
>>>
>>> Relative to memory constrains. I implemented initial version
>>> it can
>>> catch simplest cases (from test-case):
>>>
>>> @a = internal global i16 0, align 4
>>> @b = internal global i16 0, align 4
>>> define void @mem() {
>>> ;CHECK: some_instr Z, Y
>>> call void asm "some_instr $0, $1", "=*Q,=*Q"(i16* @a,
>>> i16* @b)
>>> ret void
>>> }
>>>
>>> The patch is attached.
>>>
>>> Its in my todo yet, to handle local variables. They could be
>>> emitted as
>>> Y+q expression. Hope to present this support tomorrow. So if
>>> 'a' and 'b'
>>> from example above would be local we could get "some_instr
>>> Y, Y+2"
>>>
>>> -Stepan.
>>>
>>>
>>>
>>>
>>>
>>>
>>
>
|
|
From: Borja F. <bor...@gm...> - 2013-06-22 11:39:47
|
Resending the email to the list without any previous emails: Hello Stepan, 1) This should be fixed now with my last commit. 2) I dont really understand what you mean. Can you expand a bit more here? |
|
From: Borja F. <bor...@gm...> - 2013-06-25 14:22:24
|
Ahh, I didn't know avr-gcc was buggy on this feature, that's probably the
reason for why the code i pasted in my previous email didn't work.
About the getPointerRegClass function, what is it used for? Also, please
move the implementation to the cpp file instead of leaving it in the .h.
Ok tests look good, looking at what you covered is there anything else that
needs to be implemented for this constraint?
What's wrong with clang? I fixed clang inline asm support back on friday,
so is there anything else that is broken?
2013/6/25 Stepan Dyatkovskiy <stp...@na...>
> Hi Borja.
> This is new patch. Seems I've moved memory constraint implementation to
> the same level like all other constraints. Note avr-gcc has buggy
> implementation of memory constraint, that's why *avr-libc doesn't use it*.
>
> I removed restriction for Y,Z in getLargestLegalSuperClass, so these
> registers could be inflated now. But there was unimplemented
> getPointerRegClass method in AVRRegistersInfo. So currently I have
> restricted it to Y and Z. Though suppose we can extend it in future to XYZ
> set.
> Look changes in inline-asm.ll to see what exactly is supported now.
>
> About clang.
> May be we ask John Myers to fix clang support for inline asm?
>
> -Stepan.
>
>
> Stepan Dyatkovskiy wrote:
>
>> The patch. Forgot to attach it.
>> -Stepan.
>> Stepan Dyatkovskiy wrote:
>>
>>> Hi Borja,
>>>
>>> > What happens in your example above when you use a real instruction
>>> like
>>>
>>>> for example LDD? Does it still use GPR8 regs? To me it's weird that if
>>>> you use a real instruction where operands are clearly defined in the td
>>>> file the instruction selector uses an invalid regclass.
>>>>
>>>
>>> There are two kinds of inline asm support in LLVM:
>>> 1. You just support all the constraints and pastes inline-asm contents
>>> as-is. That's why its still allowed to use names like "some_instr".
>>> 2. You may expand inline-asm strings set, or in another words just parse
>>> it onto set of instructions. In this case you have to implement
>>> TargetLowering::**ExpandInlineAsm method.
>>> I'd want to start it on this week though...
>>>
>>> But first we have to get avr-libc compilable (perhpas I read you
>>> thoughts ;-) )
>>>
>>> Relative to memory constrains. I implemented initial version it can
>>> catch simplest cases (from test-case):
>>>
>>> @a = internal global i16 0, align 4
>>> @b = internal global i16 0, align 4
>>> define void @mem() {
>>> ;CHECK: some_instr Z, Y
>>> call void asm "some_instr $0, $1", "=*Q,=*Q"(i16* @a, i16* @b)
>>> ret void
>>> }
>>>
>>> The patch is attached.
>>>
>>> Its in my todo yet, to handle local variables. They could be emitted as
>>> Y+q expression. Hope to present this support tomorrow. So if 'a' and 'b'
>>> from example above would be local we could get "some_instr Y, Y+2"
>>>
>>> -Stepan.
>>>
>>>
>>>
>>
>
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X