- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
          [AVR] Change half to use softPromoteHalfType
          #152783
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
           This is based on top of #152708 which should land first, the second commit here is the relevant part. @benshi001 could you review this as well? Cc @Patryk27  | 
    
        
          
                llvm/test/CodeGen/AVR/half.ll
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unclear about where the existing CC came from but I think it assumed that the f16 was stored in the lower half of an f32, which would be passed in r22..=r25. Now it is just using the i16 ABI, which makes more sense.
        
          
                llvm/test/CodeGen/AVR/half.ll
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, kinda funny we first store the upper-byte and then the lower-byte, didn't realize it before (doesn't seem to be a bug though 🤔)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @benshi001 as well
31904ba    to
    fabf0e6      
    Compare
  
    | 
           AVR is a quite simple 8-bit CPU, it lacks HW float unit and vector unit. In real production environment, clang-AVR relies on libgcc-7.3, which also lacks infrastrure of float16. In fact avr-gcc 7.3 even attributes double to float, as indicated in https://gcc.gnu.org/wiki/avr-gcc. So I think the best way is also attributing half type to float32, to fit avr-gcc-7.3 (and its libgcc) which is the main stream version in real production environment. However we can not prevent hand writing   | 
    
| 
           I just replied at #97975 but I'll give some more details here: 
 For context, I'm working on Rust's  Currently for AVR, you can use  
 You are talking about the Clang frontend here right, something like C23   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The default `half` legalization has some issues with quieting NaNs and carrying excess precision. As has been done for various other targets, update AVR to use `softPromoteHalfType` which avoids these issues. The most obvious corrected test below is `test_load_store`, which no longer contains calls to extend and trunc (which would cause roundtripping to fail).
fabf0e6    to
    4e3ca49      
    Compare
  
    
          
 I saw pairs of generated after this PR, though this is less efficient, it is better than broken.  | 
    
| 
           Thank you for reviewing! 
 Indeed: this changes from "unusably broken+slow" to "working but possibly slower". Really this just makes it possible to do future work. 
  | 
    
The default
halflegalization has some issues with quieting NaNs and carrying excess precision. As has been done for various other targets, update AVR to usesoftPromoteHalfTypewhich avoids these issues.The most obvious corrected test below is
test_load_store, which no longer contains calls to extend and trunc (this passing through libcalls means thatf16does not round trip).Fixes the AVR part of #97975
Fixes the AVR part of #97981