[PATCH] D140478: [RISCV] For Dwarf v5, emit indices into .debug_addr for range list entries

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu marked an inline comment as done.
RamNalamothu added a comment.

@dblaikie

As I already replied earlier, there is not enough context to reach the MachineFunction from the symbol or determine if relaxation is enabled for the given address range.

Also, at the moment, the `-mrelax/-mno-relax` affects the entire compilation unit for the RISC-V. If that changes in future, I guess it will need many other changes as well and this will also get looked at then.

@craig.topper @asb

The latest update has a target hook, which is specialized for RISC-V, and the changes are effective for only RISC-V target.
Does this look good to go now?



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:456-461
+  // The symbol's address range will only be known after the linking when using
+  // relaxations. Hence, generate address range offsets as relocatable symbols
+  // and not as absolute delta from label difference as they might get resolved
+  // before linking.
+  virtual bool needRelocSymbolsForAddrRange() const { return false; }
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Does it make sense to forward-compat this by providing the two labels to this function, even if they're ignored for now? Is it plausible that some backend could determine whether a given range is relaxable or not from those labels? Guess we can always add that in later... 
> > > 
> > > Maybe the name could be more about relaxation? "is this address range a compile-time constant" or "is this address range not subject to linker relaxation"?
> > At least I think this function should take the MachineFunction to operate on, rather than using the one that might be set on the AsmPrinter - I would guess if you test this with a module with some functions with relaxation and some without, the current implementation doesn't get it correct - because we emit all these ranges together, so the AsmPrinter's current MF, if it has one, won't be correct for all of the ranges being emitted, right?
> Which is to say - please include some test coverage showing a mixed module, demonstrating that the appropriate encodings are used for the appropriate functions (relocatable for relaxable functions, and non-relocatable for non-relaxable functions)
> Does it make sense to forward-compat this by providing the two labels to this function, even if they're ignored for now? Is it plausible that some backend could determine whether a given range is relaxable or not from those labels? Guess we can always add that in later... 

Yeah, it's a simple change that can be added when needed.

> Maybe the name could be more about relaxation? "is this address range a compile-time constant" or "is this address range not subject to linker relaxation"?

I am not getting ideas here. Feel free to suggest a name.




================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:254
+                         .getValueAsString();
+      if (FS.contains("-relax"))
+        return false;
----------------
dblaikie wrote:
> Is this possible, even when the MachineFunction's subtarget has the relax feature? (I'd have thought the subtarget would already reflect/account for/be derived from the function's attributes?)
I don't know for sure. I think what you said is true.

Will change the checks accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140478