[PATCH] Add sign/zero extend to mips fast-isel

Chad Rosier mcrosier at codeaurora.org
Thu Aug 28 18:42:06 PDT 2014


>>! In D4827#16, @dsanders wrote:
>>>! In D4827#14, @mcrosier wrote:
>>>>! In D4827#12, @rkotler wrote:
>>> I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.
>> 
>> I very much agree with Reid.

And now I recant my statement.  That's what I get for doing a drive-by review.  Thank you for the clarification, Daniel.

> 
> I disagree but to a greater or lesser extent according to the change.  I'll reply to each one inline with the reason for the request but the overarching principle is that I'd prefer to not to leave gaps in the implementation that need rediscovering at a later point since this has been a major source of pain in both the integrated assembler and code generator. The use of SEB/SEH (which requires MIPS32r2/MIPS64r2) without handling MIPS32r1/MIPS64r1 and earlier is a good example of this.

I didn't realized you were trying to support multiple implementations of the ISA.  With that understanding, I'll now back Daniel (strongly).  The implementation is incomplete.

> 
>>> Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.
>> 
>> I don't think this is a serious blocker.
> 
> Indeed, it's not a serious blocker for an individual patch. I brought it up because I'd prefer that our test cases omit the unnecessary artifacts that clang introduces for its own convenience.

Very understandable.

http://reviews.llvm.org/D4827






More information about the llvm-commits mailing list